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-03 11:59:44

On Mon, Apr 3, 2017 at 4:46 AM, Niall Douglas via Boost
<boost_at_[hidden]> 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