On Mon, Apr 3, 2017 at 4:46 AM, Niall Douglas via Boost wrote:

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. Thank you for your time and suggestions. Barrett [1] https://github.com/badair/eraserface

On Mon, Apr 3, 2017 at 9:13 AM, Peter Dimov via Boost wrote:

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. Barrett

On Mon, Apr 3, 2017 at 6:06 AM, Tim Song via Boost 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::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 is true but not is_volatile_member.

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 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. Thanks for the great feedback, Tim.

On Apr 3, 2017, at 9:30 AM, Peter Dimov via Boost wrote:

Tim Song wrote:

...

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

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). However, in C++14, variable templates have external linkage, so `inline` is not necessary for variable templates.

On Mon, Apr 3, 2017 at 12:20 PM, Peter Dimov via Boost 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).

On Mon, Apr 3, 2017 at 1:09 PM, Peter Dimov via Boost wrote:

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.

On Apr 3, 2017, at 1:34 PM, Peter Dimov via Boost 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.

On 5/04/2017 16:13, Barrett Adair wrote:

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?

On Sun, Apr 9, 2017 at 6:50 PM, Barrett Adair via Boost < boost@lists.boost.org> wrote:

On Sun, Apr 9, 2017 at 10:16 AM, Bruno Dutra via Boost

...

* Why does qualified_parent_class_of return a ref qualified type even

for

...

unqualified PMFs? This is again surprising, I would expect qualified_parent_class_of_t to return foo, not foo&.

parent_class_of has this behavior. qualified_parent_class_of attempts to address the use case of using the class as a function parameter, where foo& or foo const & would often be preferred over foo.

...

* Why does qualified_parent_class_of always return a const& qualified type for PMDs? I can imagine that this would be quite an annoyance if the user wants to apply custom qualifiers to it for one reason or another, especially rvalue reference, which would be silently collapsed away by

I understand the use-case, but consider the fact that the unqualified version should be able to work with both lvalue qualified as well as rvalue qualified `this`, therefore there doesn't seem to be a reason to prefer the lvalue qualifier. For the use-case you mentioned, the user could easily get away with it by appending the desired qualifier to qualified_parent_class_of_t<...>, i.e. & or &&, and rely on collapsing to make it consistent. the

...

lvalue reference. The safest choice IMO would be to return an unqualified type.

The idea of qualified_parent_class_of is to copy cv/ref from the member function to the class. This doesn't really make sense for PMDs, so maybe PMDs should cause a substitution error here. parent_class_of can be used for the unqualified type.

...

* The documentation explicitly states that the violation of

...

for transformation traits produces a SFINAE-friendly substitution error. While that does seem to be true for their eager *_t versions as fair as I could verify, it can't possibly hold true for the lazy versions of transformation traits, which is a direct consequence of the way they are defined, namely

template<typename T> struct trait { using type = trait_t<T>; };

This always produces a hard error if the pre-conditions for trait_t are violated.

One option would be to clearly state in the documentation to what extent transformation traits are SFINAE-friendly, however I deem it surprising that the eager versions should be SFINAE friendly whereas lazy versions are not, therefore I suggest that the definition of lazy transformation

That's probably a good idea. pre-conditions traits

...

be amended to guarantee this property as well, possibly by simply explicitly triggering a substitution error like follows

template struct trait {};

template<typename T> struct trait> { using type = trait_t<T>; };

* The above is also true for predicate traits, which define a nested typename type in terms of an unfriendly expression, instead of inheriting it along with the integral constant value. This is less of a problem in this case, since the user is most interested in the nested integral constant value, but still there doesn't seem to be any reason for the nested typename type to not be SFINAE-friendly.

Do you think it would be better to rename foo_t to foo and remove the "lazy" traits entirely? I came close to doing this for the reasons you mention above.

On one hand I totally understand your frustration with lazy metafunctions, but on the other hand I feel that Callable Traits should try to integrate seamlessly with type traits provided by the standard library to ease adoption and I think it's worth the trouble to provide them. If you are feeling uneasy with the solution I suggested in the previous email, you could also invert things and define the eager versions in terms of their lazy counterparts, but that would probably require more refactoring so as to have lazy traits somehow derive from the SFINAE friendly implementation details, so that they also "inherit SFINAE-friendliness" along with the presence of a nested typename type or conversely the lack thereof.

...

I missed a Quick Start section that helps the user set up a minimal working example, even if that would be trivial in this case.

Can you elaborate on what you mean by "minimal working example"? I plan to add more "motivating examples" and comparison code with Boost.FunctionTypes, however I think the documentation is riddled with what I would describe as minimal working examples.

Maybe "example" wasn't the best choice for a word. I was referring to a minimal example on how to *use* the library, that is a small section that tells the user what headers to include and, if the library supports being used outside of the boost distribution, how to get a copy of it and set up the project to find the correct headers. This is all trivial for a header only library, but many users genuinely wouldn't know where to start.

...

It seems only eager versions of traits are tested, I'd like to see the lazy versions tested as well.

Hmm, fair point. This makes me more inclined to remove the lazy versions. The more that I think about the lazy versions, the more I think they are useless.

Normally I would totally agree, but in this case I really feel it's important that Callable Traits follow the conventions of the standard library.

...

Also I didn't find tests that ensure SFINAE-friendly substitution errors are produced instead of hard errors, those should be added since this behavior is emphasized.

Actually, I think the test coverage is pretty good here. SFINAE tests can be found in the *_constraints.cpp files, such as

https://github.com/badair/callable_traits/blob/master/test/arg_at_constraint... Somehow I missed those. Looks fine, sorry for the noise. Bruno

Callable Traits solves an important problem. FWIW my Synapse library ( http://zajo.github.io/boost-synapse/) is an example of having to deal "manually" with the problem callable traits solves, and I do plan to make the conversion to using callable traits if it is accepted. The documentation seems complete. I did not study the implementation, but the unit tests are exhaustive and precise, very solid. I think Callable Traits should be accepted. Emil On Mon, Apr 10, 2017 at 10:47 AM, Louis Dionne via Boost < boost@lists.boost.org> wrote:

...

Dear Boost community,

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

There's just two days left for the review, and so far I count 3 formal reviews:

Bruno Dutra Klemens Morgenstern Zach Laine

There's been other useful feedback too which I will take into account, but not in the form of formal reviews with a clear yes/no vote. I'd like to encourage more people to submit a formal review, as that will make my decision much easier to make and justify.

Thanks, Louis

On Wed, Apr 12, 2017 at 7:08 AM, Fletcher, John P via Boost wrote:

I have only time to do a very brief evaluation.

I have tried this out with Clang 3.8

There are a few things which run with C++14 and not C++11 in the example code in the documentation:

static_assert(ct::has_void_return_v<foo>, "");

static_assert(!ct::has_varargs_v<foo>, "");

static_assert(ct::is_const_member_v<pmf>, "");

I can understand the intention from the documentation.

I support adding this to Boost.

Best wishes

John

Thanks, John. I should remove the variable templates from the examples so that C++11 users don't experience confusion.

_______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost