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