[CallableTraits] The formal review begins today

[CallableTraits] The formal review begins today

Dear Boost community,

The formal review of Barrett Adair's CallableTraits library begins today,
April 3rd, and ends on April 12th.

CallableTraits is a C++11 library for the inspection, synthesis, and
decomposition of callable types. CallableTraits aims to be the "complete
type manipulation facility for function types" mentioned in the final
paragraph of C++17 proposal P0172, and removes the need for template
specializations for different function signatures.

We encourage your participation in this review. At a minimum, kindly state:
- Whether you believe the library should be accepted into Boost, and
conditions for acceptance if any
- Your name
- Your knowledge of the problem domain

You are strongly encouraged to also provide additional information:
- What is your evaluation of the library's:
* Design
* Implementation
* Documentation
* Tests
* Usefulness
- Did you attempt to use the library? If so:
* Which compiler(s)?
* What was the experience? Any problems?
- How much effort did you put into your evaluation of the review?

* The fact this library can be used without the rest of Boost needs much
more obvious declaration

* Why is a std::tuple type the way of returning the function signature?
Why not some arbitrary (user supplied) variadic template?

* I would really prefer a dedicated Compatibility page with a matrix of
tested compiler versions against each of the facilities. Right now I
need to find the correct function to use and dig into its reference
docs. This is unhelpful when I am evaluating the viability of whether to
use CallableTraits or not.

* I can't help but think repeatedly that Concepts would be real useful
in this library. I was surprised they were not mentioned. In particular
the docs say things like "If cannot be legally instantiated according to
the behaviour above, the behaviour is undefined". I would much prefer
that failure to instantiate is a concept failure and/or a static assert
or other useful compiler diagnostic, except where SFINAE is desired. In
other words, can I not choose which behaviour I want?

* Some ABIs let you add proprietary qualifiers to function types e.g.
__stdcall. I see only a tiny mention of this at the bottom of the FAQ,
yet dealing with proprietary function signature semantics is unavoidable
in those systems e.g. introspecting a user supplied pointer to a Win32
syscall.

Re: [CallableTraits] The formal review begins today

I don't think I've participated on this list enough to do a formal
review, but here are some comments from a first look:

- The documentation misuses the term "implementation-defined"
throughout; "see below" seems to be closer to what is intended.

- Technically, the fallback definitions of foo_v are all ill-formed NDR.

- The description of behavior seems a bit awkward due to the use of
the passive tense: "std::false_type is inherited by
is_lvalue_reference_member<T> and is aliased by typename
is_lvalue_reference_member<T>::type, except when one of the following
criteria is met, in which case std::true_type would be similarly
inherited and aliased". Examining the implementation, it is unclear
why the repeated alias declaration is necessary when false/true_type
already define a member typedef 'type' that is itself.

- The traits do not, but should, handle top-level cv-qualification. I
see no reason why, say, is_volatile_member<int (foo::*)() volatile> is
true but not is_volatile_member<int (foo::* const)() volatile>.

- "reference collapsing" behavior on the add ref-qualifier traits is
subtle and arbitrary and I'm not aware of use cases calling for such
rules. The FAQ's argument - that a different set of rules would be
hard to memorize - does not sound very convincing to me. It sounds
like the problem came from the name of the trait: "adding" a &&
qualifier to a &-qualified function type doesn't make much sense. If,
say, the name is recast as "make the function type && qualified", then
the behavior would be obvious.

- Speaking of names, I'm not a big fan of names like
"is_reference_member" for a trait that's actually "has ref-qualifier".
Likewise for something like "is_volatile_member"; my first reaction is
"is it a pointer to member data of volatile-qualified type?". Maybe
the namespace helps, I don't know.

- Consider making the variable templates inline for implementations
that support it, to match C++17.

- apply_member_pointer seems to be trying to do too much. I can get a
pointer to member data of any type, unless that data member is itself
a pointer to member or a pointer to function, in which case I get
something else entirely. Additionally, the description doesn't mention
that PMDs also get special handling.

- transaction_safe is not a "C++17 feature".

- Is there an explanation for qualified_parent_class_of's design,
especially with respect to PMDs (the choice of const &)?

Re: [CallableTraits] The formal review begins today

> On 03/04/2017 07:45, Louis Dionne via Boost wrote:
>> Dear Boost community,
>>
>> The formal review of Barrett Adair's CallableTraits library begins today,
>> April 3rd, and ends on April 12th.
>
> First impressions of the docs: Looks pretty good, but:
>
> * The fact this library can be used without the rest of Boost needs much
> more obvious declaration

Hmm, good point.

> * Why is a std::tuple type the way of returning the function signature?

It's only used for `args` (and as special case input for `apply_return`).
I thought this was useful.

> Why not some arbitrary (user supplied) variadic template?

`expand_args` sounds like what you're describing.

> * I would really prefer a dedicated Compatibility page with a matrix of
> tested compiler versions against each of the facilities. Right now I
> need to find the correct function to use and dig into its reference
> docs. This is unhelpful when I am evaluating the viability of whether to
> use CallableTraits or not.

Noted. I originally had one of these pages, but I decided to remove it,
because I thought that most users of this library would only need one
or two features at a time.

Generally speaking, the library is usable with GCC 4.7.3+, Clang 3.5.2+,
and Visual Studio 2015+. Intel C++ 2017 also kind of works, but this
would be fixed/tested/documented fully if accepted.

> * I can't help but think repeatedly that Concepts would be real useful
> in this library. I was surprised they were not mentioned. In particular
> the docs say things like "If cannot be legally instantiated according to
> the behaviour above, the behaviour is undefined". I would much prefer
> that failure to instantiate is a concept failure and/or a static assert
> or other useful compiler diagnostic, except where SFINAE is desired. In
> other words, can I not choose which behaviour I want?

I can't prevent users from static_assert-ing (or worse) inside their own
templates when the library tries to instantiate them. The "behavior is
undefined" notes are limited to the features with template template
parameters. Everything else can only fail during substitution. However,
I took some pains to use error messages as type names, such that a
substitution failure outside of a SFINAE context still provides the same
degree of verbosity that the library would have had it used static_assert
instead.

I made the decision to use SFINAE over static_assert while using
CallableTraits to implement Eraserface [1]. I think this was the right
decision. I see the benefits of static_assert, but I think they are
significantly outweighed by the "SFINAE errors" approach used here,
especially since these features are likely to be used in a context
where SFINAE applies.

Concepts might be nice, but this was not an option, because I chose to
support a wide range of compilers.

> * Some ABIs let you add proprietary qualifiers to function types e.g.
> __stdcall. I see only a tiny mention of this at the bottom of the FAQ,
> yet dealing with proprietary function signature semantics is unavoidable
> in those systems e.g. introspecting a user supplied pointer to a Win32
> syscall.
>

Indeed. I originally had support for __stdcall, __fastcall, __cdecl,
and pascal. There is still vestigial code for this, because I thought it
would come up during review. I decided that the test surface for these
features across compiler versions was larger than I wanted to bite
off for the first iteration of the library. It's very time consuming to do
that correctly, so I wanted to wait and see if it would be a desired feature.
The library does correctly handle the __cdecl qualifier on MSVC varargs
member functions, though. Support for the rest wouldn't be added quickly,
but it could be done.

Re: [CallableTraits] The formal review begins today

On Mon, Apr 3, 2017 at 6:06 AM, Tim Song via Boost
<[hidden email]> wrote:
> I don't think I've participated on this list enough to do a formal
> review, but here are some comments from a first look:
>
> - The documentation misuses the term "implementation-defined"
> throughout; "see below" seems to be closer to what is intended.
>

Agreed... "see below" would be better.

> - Technically, the fallback definitions of foo_v are all ill-formed NDR.

True, but in practice does this actually matter? I decided that this
behavior would be more user-friendly than simply removing foo_v
entirely. Is `std::is_same<T, some_hidden_type>::value`
preferred for cases like this?

> - The description of behavior seems a bit awkward due to the use of
> the passive tense: "std::false_type is inherited by
> is_lvalue_reference_member<T> and is aliased by typename
> is_lvalue_reference_member<T>::type, except when one of the following
> criteria is met, in which case std::true_type would be similarly
> inherited and aliased". Examining the implementation, it is unclear
> why the repeated alias declaration is necessary when false/true_type
> already define a member typedef 'type' that is itself.
>

Agreed. This should definitely be reworded throughout.

> - The traits do not, but should, handle top-level cv-qualification. I
> see no reason why, say, is_volatile_member<int (foo::*)() volatile> is
> true but not is_volatile_member<int (foo::* const)() volatile>.

I originally handled top-level CV. I thought that ignoring top-level CV
made the library "leaner" and easier to document, but I didn't feel
strongly about that decision. This would be easy to change.

> - "reference collapsing" behavior on the add ref-qualifier traits is
> subtle and arbitrary and I'm not aware of use cases calling for such
> rules. The FAQ's argument - that a different set of rules would be
> hard to memorize - does not sound very convincing to me. It sounds
> like the problem came from the name of the trait: "adding" a &&
> qualifier to a &-qualified function type doesn't make much sense. If,
> say, the name is recast as "make the function type && qualified", then
> the behavior would be obvious.

Fair point... I had to choose a scheme, so I went with reference
collapsing. I did try to design the library to parallel <type_traits> for the
sake of familiarity, but maybe reference collapsing goes too far.
Your suggestion makes sense to me.

> - Speaking of names, I'm not a big fan of names like
> "is_reference_member" for a trait that's actually "has ref-qualifier".
> Likewise for something like "is_volatile_member"; my first reaction is
> "is it a pointer to member data of volatile-qualified type?". Maybe
> the namespace helps, I don't know.

I really spent a lot of time thinking about names, and I was never
truly satisfied. I do like this suggestion.

> - Consider making the variable templates inline for implementations
> that support it, to match C++17.

Good catch.

> - apply_member_pointer seems to be trying to do too much. I can get a
> pointer to member data of any type, unless that data member is itself
> a pointer to member or a pointer to function, in which case I get
> something else entirely. Additionally, the description doesn't mention
> that PMDs also get special handling.

Fair point. Maybe this feature should be removed.

> - transaction_safe is not a "C++17 feature".

Yes, this was a documentation mistake that needs to be fixed.

> - Is there an explanation for qualified_parent_class_of's design,
> especially with respect to PMDs (the choice of const &)?

I found it useful, so I threw it in the library. This feature is easier
to use than combining all the `is_foo` + `add_foo` features together
with `std::conditional`. I went back and forth on the PMD case.
I ended up with `const` because it worked best for my use case.

Re: [CallableTraits] The formal review begins today

Paul Fultz II wrote:
> On Apr 3, 2017, at 9:30 AM, Peter Dimov via Boost <[hidden email]>
> wrote:
> > constexpr variables are inline by default if I'm not mistaken.
>
> No they are not, it was considered at one point(which would have been
> nice).

Re: [CallableTraits] The formal review begins today

Tim Song wrote:
> >> However, in C++14, variable templates have external linkage, so
> >> `inline` is not necessary for variable templates.
> >
> > Indeed. They're templates after all.
>
> As I understand it, this depends on Core issue 1713, whose unresolved
> status presumably led LWG to go with putting inline on everything at Kona
> (see P0607R0; LWG moved A and B2).

It makes no sense for variable templates to "not be inline", because that
would make them useless. Templates are defined in headers. They have to be
"inline" in the same way template functions have to be "inline" even when
they aren't.

Re: [CallableTraits] The formal review begins today

> Tim Song wrote:
>>
>> >> However, in C++14, variable templates have external linkage, so >>
>> >> `inline` is not necessary for variable templates.
>> >
>> > Indeed. They're templates after all.
>>
>> As I understand it, this depends on Core issue 1713, whose unresolved
>> status presumably led LWG to go with putting inline on everything at Kona
>> (see P0607R0; LWG moved A and B2).
>
>
> It makes no sense for variable templates to "not be inline", because that
> would make them useless. Templates are defined in headers. They have to be
> "inline" in the same way template functions have to be "inline" even when
> they aren't.
>

The issue was, I believe, whether the const implied by the constexpr
gave the variable template internal linkage; even though they are
templates, that doesn't help with the ODR problem if they are actually
different templates.

Re: [CallableTraits] The formal review begins today

Tim Song wrote:

> The issue was, I believe, whether the const implied by the constexpr gave
> the variable template internal linkage; even though they are templates,
> that doesn't help with the ODR problem if they are actually different
> templates.

Yes, it would be similar to the (mostly theoretical) problem created by _1,
if passed by reference to a template. I doubt that any implementation gives
variable template instantiations internal linkage though. It just makes no
sense. I don't know why issue 1713 hasn't been resolved yet.

Re: [CallableTraits] The formal review begins today

> On Apr 3, 2017, at 1:34 PM, Peter Dimov via Boost <[hidden email]> wrote:
>
> Tim Song wrote:
>
>> The issue was, I believe, whether the const implied by the constexpr gave the variable template internal linkage; even though they are templates, that doesn't help with the ODR problem if they are actually different templates.
>
> Yes, it would be similar to the (mostly theoretical) problem created by _1, if passed by reference to a template.

Its more than theoretical, with internal linkage it will bloat the binary with duplicate symbols. Most linkers can remove these duplicates, but do not always and are not required to.

Re: [CallableTraits] The formal review begins today

> Dear Boost community,
>
> The formal review of Barrett Adair's CallableTraits library begins today,
> April 3rd, and ends on April 12th.
>
> CallableTraits is a C++11 library for the inspection, synthesis, and
> decomposition of callable types. CallableTraits aims to be the "complete
> type manipulation facility for function types" mentioned in the final
> paragraph of C++17 proposal P0172, and removes the need for template
> specializations for different function signatures.
>
> You can find the documentation of the library here:
>
> http://badair.github.io/callable_traits/doc/html/index.html>
> and the GitHub repository here:
>
> https://github.com/badair/callable_traits>
> We encourage your participation in this review. At a minimum, kindly state:
> - Whether you believe the library should be accepted into Boost, and
> conditions for acceptance if any

>
> You are strongly encouraged to also provide additional information:
> - What is your evaluation of the library's:
> * Design
Seems fine, though I have a few remarks:

I would've expected a make_fn and make_mem_fn function, with which one
could construct a type from a return type and an std::tuple of the
arguments. That can be easily done manually but I would've expected that
in this library.

Additionally I felt, that there are a few to many algorithms, I would've
expected the library to only provide an argument tuple so I can
manipulate that with fusion, hana or whatever. Now I guess tha variadic
C parameter is the reason, but that can only be pushed to the end
anyway. So I'm not sure why the library would've just go with std::tuple
and let some other tool do the rest.

> * Implementation
Didn't check.
> * Documentation
Seems fine to me. Though there's one thing that needs to be adressed,
which is: what about overloaded functions and more importantly,
overloaded operator()?

> * Tests
Looked at a few files, seem solid.
> * Usefulness
Very useful, that really safes much boiler-plate code. Given the
complexity it makes sense to have a custom library instead of adding it
to type_traits.
> - Did you attempt to use the library?
No
> - How much effort did you put into your evaluation of the review?
About one hour, reading the doc looking at a few tests.

Re: [CallableTraits] The formal review begins today

> Am 03.04.2017 um 08:45 schrieb Louis Dionne via Boost:
>>
>> [snip]
>>
>> We encourage your participation in this review. At a minimum, kindly
>> state:
>> - Whether you believe the library should be accepted into Boost, and
>> conditions for acceptance if any
>
> Yes it should be.
>
>> - Your name
>
> Klemens Morgenstern
>>
>> - Your knowledge of the problem domain
>
>
> I've implemented similar things by hand several times,including in boost.dll
> (https://github.com/apolukhin/Boost.DLL/blob/develop/include/boost/dll/detail/get_mem_fn_type.hpp)
>
>>
>> You are strongly encouraged to also provide additional information:
>> - What is your evaluation of the library's:
>> * Design
>
> Seems fine, though I have a few remarks:
>
> I would've expected a make_fn and make_mem_fn function, with which one could
> construct a type from a return type and an std::tuple of the arguments. That
> can be easily done manually but I would've expected that in this library.
>

`apply_return` has a special case for this, but there isn't a member
function equivalent. I will consider implementing this.

> Additionally I felt, that there are a few to many algorithms, I would've
> expected the library to only provide an argument tuple so I can manipulate
> that with fusion, hana or whatever. Now I guess tha variadic C parameter is
> the reason, but that can only be pushed to the end anyway. So I'm not sure
> why the library would've just go with std::tuple and let some other tool do
> the rest.
>

Good point. These were some of the last features I added to the library, and
perhaps the most indulgent. I'm not opposed to removing them. The presence
of these algorithms make the preprocessed header length quite large, which
is my main gripe with my implementation. I'd hoped to merge the "cleanup"
branch before the review, which is more optimized for this, but it
still generates
nearly 20k lines of source code with GCC 6.3/c++1z (before counting std
headers). Removing the algorithms will yield a very large reduction in
code size.

I played around with calling convention support quite a bit before ultimately
removing most of that code. I wanted to get the rest of the library
implemented, and
then come back and add calling conventions in a later version if the
initial version
is well-received by the community, and if there is demand for these
features. I wasn't
prepared to invest the time to test all the possible combinations of calling
conventions and qualifiers before knowing whether the library would ever see the
light of day.

There is still support for the default __cdecl that MSVC uses for
member functions
with C-style variadics, and much of the code is still in place to
support other calling
conventions.

When writing the docs, I decided that function overloads weren't really worth
mentioning, since one can't decltype them without casting. They did not seem
relevant, much in the same way that inlining does not seem relevant. Perhaps
I should add a paragraph describing the parts of a function that do not concern
CallableTraits -- overloads, inlining, definitions, default
parameters, parameter
names, attributes, address, etc.

operator() overloads, on the other hand, are mentioned pervasively in
the docs --
these should always trigger substitution failures.

>> * Tests
>
> Looked at a few files, seem solid.
>>
>> * Usefulness
>
> Very useful, that really safes much boiler-plate code. Given the complexity
> it makes sense to have a custom library instead of adding it to type_traits.
>>
>> - Did you attempt to use the library?
>
> No
>>
>> - How much effort did you put into your evaluation of the review?
>
> About one hour, reading the doc looking at a few tests.
>

Re: [CallableTraits] The formal review begins today

> Barrett Adair wrote:
>
>> The "behavior is undefined" notes are limited to the features with
>> template template parameters.
>
>
> This seems wrong. Is the behavior really undefined here? "It could format
> your hard drive" undefined? It seems to me that the worst that can happen is
> a compile error ("the program is ill-formed").
>

No, it's not "Undefined Behavior" in the ISO C++ Standard sense. I understood
"undefined behavior" in the context of a non-std library to mean "violation of
contract", but probably this is a misleading use of the term. Indeed, a compile
error is the worst that can happen.

In retrospect, it's likely better to remove these warnings entirely -- they
are really just disclaimers for the case where a user template e.g. fails a
static_assert, and a CallableTraits feature appears to be unable to SFINAE
as promised.

Re: [CallableTraits] The formal review begins today

> On Tue, Apr 4, 2017 at 11:52 AM, Klemens Morgenstern wrote:
>> Additionally I felt, that there are a few to many algorithms, I would've
>> expected the library to only provide an argument tuple so I can manipulate
>> that with fusion, hana or whatever. Now I guess tha variadic C parameter is
>> the reason, but that can only be pushed to the end anyway. So I'm not sure
>> why the library would've just go with std::tuple and let some other tool do
>> the rest.
>
> Good point. These were some of the last features I added to the library, and
> perhaps the most indulgent. I'm not opposed to removing them. The presence
> of these algorithms make the preprocessed header length quite large, which
> is my main gripe with my implementation. I'd hoped to merge the "cleanup"
> branch before the review, which is more optimized for this, but it
> still generates
> nearly 20k lines of source code with GCC 6.3/c++1z (before counting std
> headers). Removing the algorithms will yield a very large reduction in
> code size.

Perhaps add some example documentation for equivalent usage of those
other libraries for common usage patterns with CallableTraits, for the
people who want to use CallableTraits but are less familiar with the
other libraries?

Re: [CallableTraits] The formal review begins today

Barrett Adair wrote:
> On Tue, Apr 4, 2017 at 11:52 AM, Klemens Morgenstern via Boost
> <[hidden email]> wrote:
...
> > Additionally I felt, that there are a few to many algorithms, I would've
> > expected the library to only provide an argument tuple so I can
> > manipulate that with fusion, hana or whatever. Now I guess tha variadic
> > C parameter is the reason, but that can only be pushed to the end
> > anyway. So I'm not sure why the library would've just go with std::tuple
> > and let some other tool do the rest.

This was also one of my first thoughts when I looked at the library - that's
another example of an ad-hoc metaprogramming library inside another library
that we hopefully can avoid at some point. But...

> Good point. These were some of the last features I added to the library,
> and perhaps the most indulgent. I'm not opposed to removing them.

... it's not as simple as that, because just getting a std::tuple isn't very
convenient in this case because of member pointers and the corresponding
first argument they add to the tuple. In some scenarios this is fine, in
others one would probably want to manipulate just the argument list.

Either way, having just the ability to replace the argument list with
another std::tuple (or another L<T...>) would probably be enough. Perhaps
provide two 'get' functions and two 'replace' functions, one that includes
the class in the tuple and one that doesn't.

The library could also define a special "varargs" type and include that in
the tuple as the last argument when the function is C-variadic.

Re: [CallableTraits] The formal review begins today

> Dear Boost community,
>
> The formal review of Barrett Adair's CallableTraits library begins today,
> April 3rd, and ends on April 12th.
>
> CallableTraits is a C++11 library for the inspection, synthesis, and
> decomposition of callable types. CallableTraits aims to be the "complete
> type manipulation facility for function types" mentioned in the final
> paragraph of C++17 proposal P0172, and removes the need for template
> specializations for different function signatures.
>
> You can find the documentation of the library here:
>
> http://badair.github.io/callable_traits/doc/html/index.html>
> and the GitHub repository here:
>
> https://github.com/badair/callable_traits>
> We encourage your participation in this review. At a minimum, kindly state:
> - Whether you believe the library should be accepted into Boost, and
> conditions for acceptance if any
> - Your name
> - Your knowledge of the problem domain
>
> You are strongly encouraged to also provide additional information:
> - What is your evaluation of the library's:
> * Design
> * Implementation
> * Documentation
> * Tests
> * Usefulness
> - Did you attempt to use the library? If so:
> * Which compiler(s)?
> * What was the experience? Any problems?
> - How much effort did you put into your evaluation of the review?
>
> We await your feedback!

The documentation is confusing regarding the level of C++ conformance
needed to use the library. At the beginning it says that the library is
a C++11/C++14/C++17 library, whatever this means. Later information
explains that it is only dependent on a C++11 standard library. There is
mention of compatibility claims, which further confuses the issue.
Finally there is mention of static asserts or substitution failures for
some functionality.

Frankly with all this confusion I would be very loath to use the library
as the doc explains the issue(s). It would be much clearer if the
library expressed the minimum level of C++ compliance needed for the
majority of the functionality and then documented any greater level of
C++ conformance needed for specific functionality. Also the library
should document a standard behavior when the level of conformance is not
met, either internal adjustment to a lower level of conformance,
compiler errors, static asserts, or run-time exceptions, depending on
how the library is designed, and these need to be carefully explained if
they differed for different functionality.

The number 3) item in comparing callable traits to the current Boost
function types needs more explanation. Having used Boost function types
in code ( Boost tti uses it, and I have used it in other programming
endeavors ) I do not see how the callable traits library author could
think that Boost function types encourages template specializations, so
maybe an example showing what is meant by item 3) would be needed to
back up this claim. Please note: I am certainly not against modernizing
the function type interface; I just think that claims vis a vis Boost
function types need to be backed up with actual proof by example.