[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! Regards, Louis
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 * 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. Anyway, time for the day job so I'll stop there. Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
On Mon, Apr 3, 2017 at 4:46 AM, Niall Douglas via Boost <boost@lists.boost.org> 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
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").
On Mon, Apr 3, 2017 at 9:13 AM, Peter Dimov via Boost <boost@lists.boost.org> 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
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 &)? Tim
On Mon, Apr 3, 2017 at 6:06 AM, Tim Song via Boost <boost@lists.boost.org> 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. Thanks for the great feedback, Tim.
On Apr 3, 2017, at 9:30 AM, Peter Dimov via Boost <boost@lists.boost.org> 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.
Paul Fultz II wrote:
On Apr 3, 2017, at 9:30 AM, Peter Dimov via Boost <boost@lists.boost.org> 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).
Yes, you're right, https://isocpp.org/files/papers/p0636r0.html says "Implied for static constexpr class data members."
However, in C++14, variable templates have external linkage, so `inline` is not necessary for variable templates.
Indeed. They're templates after all.
On Mon, Apr 3, 2017 at 12:20 PM, Peter Dimov via Boost <boost@lists.boost.org> 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).
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.
On Mon, Apr 3, 2017 at 1:09 PM, Peter Dimov via Boost <boost@lists.boost.org> 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.
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.
On Apr 3, 2017, at 1:34 PM, Peter Dimov via Boost <boost@lists.boost.org> 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.
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:
[...]
Thanks for the feedback so far. Please do consider submitting a formal review; it does not matter whether you've participated a lot on this mailing list, so long as you have valuable feedback and are making sound technical points. It will make my decision as the review manager easier if you submit a formal review than if you just give informal (yet very valuable) feedback. Regards, Louis -- View this message in context: http://boost.2283326.n4.nabble.com/CallableTraits-The-formal-review-begins-t... Sent from the Boost - Dev mailing list archive at Nabble.com.
Am 03.04.2017 um 08:45 schrieb Louis Dionne via Boost:
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 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...)
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. In addition, what about call conventions? Afaik those are part of the signature on windows, or at least can be. Well I had one use case where I needed them (https://github.com/apolukhin/Boost.DLL/blob/develop/include/boost/dll/detail...) but I thought I'll mention it.
* 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.
On Tue, Apr 4, 2017 at 11:52 AM, Klemens Morgenstern via Boost <boost@lists.boost.org> wrote:
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...)
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.
In addition, what about call conventions? Afaik those are part of the signature on windows, or at least can be. Well I had one use case where I needed them (https://github.com/apolukhin/Boost.DLL/blob/develop/include/boost/dll/detail...) but I thought I'll mention it.
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.
* 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()?
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.
Thank you very much for taking the time to do this review.
_______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
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?
Barrett Adair wrote:
On Tue, Apr 4, 2017 at 11:52 AM, Klemens Morgenstern via Boost <boost@lists.boost.org> 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.
On 4/3/2017 2:45 AM, 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.
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.
On Wed, Apr 5, 2017 at 6:12 PM, Edward Diener via Boost <boost@lists.boost.org> wrote:
On 4/3/2017 2:45 AM, Louis Dionne via Boost wrote:
[snip]
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.
Thanks for the excellent feedback, Edward. I agree with your criticisms. I will improve the documentation after the review to clarify these issues.
_______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
On Mon, Apr 3, 2017 at 8:45 AM, Louis Dionne via Boost < boost@lists.boost.org> wrote:
The formal review of Barrett Adair's CallableTraits library begins today, April 3rd, and ends on April 12th.
We encourage your participation in this review. At a minimum, kindly state: - Whether you believe the library should be accepted into Boost, and
Yes.
conditions for acceptance if any - Your name
Bruno Dutra
- Your knowledge of the problem domain
I've done extensive work with template metaprogramming for the past couple of years while developing Metal.
You are strongly encouraged to also provide additional information: - What is your evaluation of the library's: * Design
The design is sound overall, but I have a couple of remarks that I'd like to see addressed. * I find it very surprising that the top-level cv and ref qualifiers aren't consistently handled throughout, there is no reason why querying a property of a member function through a PMF should depend on the qualifiers of the pointer itself. * 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<void (foo::*)()> to return foo, not 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 the lvalue reference. The safest choice IMO would be to return an unqualified type. * The various algorithms on sequences feel out of place in this library, especially considering there are better and more generic options available in Boost and even in the standard library for the arguably most useful ones, namely std::tuple_element and std::tuple_size. * The traits args and expand_args would be better merged into a single binary trait, whose second template template parameter is defaulted to std::tuple. * The documentation explicitly states that the violation of pre-conditions 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 traits be amended to guarantee this property as well, possibly by simply explicitly triggering a substitution error like follows template<typename T, typename = void> struct trait {}; template<typename T> struct trait<T, std::void_t<trait_t<T>>> { 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.
* Implementation
Quickly skimmed through, looks fine.
* Documentation
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.
* Tests
It seems only eager versions of traits are tested, I'd like to see the lazy versions tested as well. 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. * Usefulness
- Did you attempt to use the library? If so: * Which compiler(s)?
Yes, on Linux using GCC trunk and also Clang trunk + libc++ trunk.
* What was the experience? Any problems?
Pleasant, no issues.
- How much effort did you put into your evaluation of the review?
Spent about 3-4 hours reading the documentation and playing with the library. Bruno
On Sun, Apr 9, 2017 at 10:16 AM, Bruno Dutra via Boost <boost@lists.boost.org> wrote:
On Mon, Apr 3, 2017 at 8:45 AM, Louis Dionne via Boost < boost@lists.boost.org> wrote:
The formal review of Barrett Adair's CallableTraits library begins today, April 3rd, and ends on April 12th.
We encourage your participation in this review. At a minimum, kindly state: - Whether you believe the library should be accepted into Boost, and
Yes.
conditions for acceptance if any - Your name
Bruno Dutra
- Your knowledge of the problem domain
I've done extensive work with template metaprogramming for the past couple of years while developing Metal.
You are strongly encouraged to also provide additional information: - What is your evaluation of the library's: * Design
The design is sound overall, but I have a couple of remarks that I'd like to see addressed.
* I find it very surprising that the top-level cv and ref qualifiers aren't consistently handled throughout, there is no reason why querying a property of a member function through a PMF should depend on the qualifiers of the pointer itself.
I went back and forth on this. I decided to consistently *not* handle them, but I'm pretty convinced that this was the wrong decision. This should be fixed.
* 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<void (foo::*)()> 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 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 various algorithms on sequences feel out of place in this library, especially considering there are better and more generic options available in Boost and even in the standard library for the arguably most useful ones, namely std::tuple_element and std::tuple_size.
I agree, these will be removed -- except for, perhaps, pop_args_front and push_args_front. These can be useful when "thunking" member functions.
* The traits args and expand_args would be better merged into a single binary trait, whose second template template parameter is defaulted to std::tuple.
Agreed, nice catch.
* The documentation explicitly states that the violation of pre-conditions 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 traits be amended to guarantee this property as well, possibly by simply explicitly triggering a substitution error like follows
template<typename T, typename = void> struct trait {};
template<typename T> struct trait<T, std::void_t<trait_t<T>>> { 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.
* Implementation
Quickly skimmed through, looks fine.
* Documentation
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.
* Tests
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.
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...
* Usefulness
- Did you attempt to use the library? If so: * Which compiler(s)?
Yes, on Linux using GCC trunk and also Clang trunk + libc++ trunk.
* What was the experience? Any problems?
Pleasant, no issues.
- How much effort did you put into your evaluation of the review?
Spent about 3-4 hours reading the documentation and playing with the library.
Bruno
Thanks for the thorough and insightful review. This has been very helpful.
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<void (foo::*)()> 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<typename T, typename = void> struct trait {};
template<typename T> struct trait<T, std::void_t<trait_t<T>>> { 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
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 -- View this message in context: http://boost.2283326.n4.nabble.com/CallableTraits-The-formal-review-begins-t... Sent from the Boost - Dev mailing list archive at Nabble.com.
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
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
On Wed, Apr 12, 2017 at 7:08 AM, Fletcher, John P via Boost <boost@lists.boost.org> 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
participants (12)
-
Barrett Adair -
Bruno Dutra -
Edward Diener -
Emil Dotchevski -
Fletcher, John P -
Gavin Lambert -
Klemens Morgenstern -
Louis Dionne -
Niall Douglas -
Paul Fultz II -
Peter Dimov -
Tim Song