Re: [boost] [CallableTraits] The formal review begins today

Subject: Re: [boost] [CallableTraits] The formal review begins today
From: Barrett Adair (barrettellisadair_at_[hidden])
Date: 2017-04-05 04:13:56

On Tue, Apr 4, 2017 at 11:52 AM, Klemens Morgenstern via Boost
<boost_at_[hidden]> 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/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.

> 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/ctor_dtor.hpp)
> 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