RFR: 8251274: Provide utility types for function SFINAE using extra template parameters

Thu Aug 27 08:24:41 UTC 2020

Hi Kim,

On 2020-08-27 07:40, Kim Barrett wrote:
>> On Aug 26, 2020, at 11:42 AM, Erik Österlund <erik.osterlund at oracle.com> wrote:
>>
>> Hi Kim,
>>
>> I have one comment on this. IMO using value or type based SFINAE is mostly a matter of preference. Either you prefer passing in trait types, or you prefer passing in constexpr booleans. The constexpr boolean approach seems to be strictly more useful IMO, because you can pass in evaluated constexpr functions, which is simply not possible in type based SFINAE. I have for example a situation where I have VM_Version support for a feature being returned in a constexpr function, that I can use to SFINAE away platform code on unsupported platforms (actual use case for me).
> IIt's easy to bounce back and forth using things like std::integral_constant.
> (One could even go the route of Boost.Hana for that sort of thing. But I
> don't think we are likely to be doing enough metaprogramming to make
> implementing that level of infrastructure worthwhile.) So I dispute the
> suggestion that one is more useful than the other.

It is also easy to write ::value when passing in a type. BTW what I 
meant with one style being more "useful" than the other... what I really 
meant to say is that the value SFINAE is strictly more powerful than 
type SFINAE in expressiveness. Specifically, you can express all type 
SFINAE with value SFINAE by just appending "::value" to the type trait 
you pass in. Conversely, you can not express all useages of value SFINAE 
with type SFINAE, due to constexpr. This makes value SFINAE strictly 
more powerful in expressiveness. Unless you write the whole constexpr 
function body as a composition of type traits of course... which I hope 
we won't do... but we kind of actually do in some places as we didn't 
have constexpr available yet. So if we are to have a convention using 
only one of the two, value SFINAE is the more desirable alternative.

>> Therefore, instead of having both EnableIfX and EnableIfT, I wonder if we could just stick to one convention instead, and say we pass in booleans, not types wrapping booleans. That way, there will never be any question as to which convention is being used or which one you should use - we use only one, and simply convert types to booleans before passing in to EnableIf. And then we don't need to have multiple EnableIf macros to differentiate the two cases. We just stick to one by convention and say this is the way we do things. Today we do not to my knowledge use any type SFINAE in HotSpot, it's all value based (the parameter to EnableIf is a bool, not a type). So it would further more seem that is the convention we already have today, and I think is the convention we want to keep on using, especially due to constexpr making that choice strictly more useful.
> We could do that. Indeed, the Standard only provides std::enable_if (and
> friends) that takes a bool condition.

Right. And that is my preference too.

> However, I think type-based SFINAE is common and likely to remain so because
> that's the form the type traits currently come in. (Though C++17 adds
> template variables for most or all of the trait types. We could perhaps
> provide our own until we can use C++17.)
>
> I find the frequent `::value` suffixes intrusive and annoying. (The
> alternative of constructing the trait type is perhaps less annoying, i.e.
> EnableIfX<std::is_integral<T>()>. Maybe that's good enough. It's also the
> same number of characters as the C++17 template variables, though less
> useful for additional calculations (unless going Boost.Hana style).)

Right. So on the one hand side you find it annoying to type "::value" 
when passing in a type trait. But on the other hand, having both 
EnableIfX and EnableIfT, and forcing readers, writers and reviewers to 
figure out which one is used or should be used, has the following also 
annoying properties:
* There is no technical need for EnableIfT, so I don't see why we need 
the extra confusion about using EnableIfT vs EnableIfX. And I think 
SFINAE code does not necessarily need to become more confusing than it 
already is to readers in HotSpot.
* Makes us not have a convention any more. Code will be written 
inconsistently sometimes randomly passing in a bool by some programmers, 
and with a type for other programmers, based on stylistic preferences 
alone, as opposed to different technical needs. I think that kind of 
stylistic choices only leads to a soup of different code styles and 
inconsistencies that didn't need to be there if we could just agree upon 
one convention instead. Such things tend to also introduce meaningless 
discussion points and hence stalls in review threads, where people argue 
that this or that style should be used due to different consistency vs 
style preference arguments and opinions. With a single convention, such 
debates are more easily shut down.
* If we are suddenly allowing type SFINAE as well, purely because we 
think it stylistically looks better, then let's discuss style. I think 
having to write EnableIfX instead of EnableIf (or similarly ENABLE_IF_X 
instead of ENABLE_IF) is stylistically ugly. So if anything, having both 
type and value SFINAE, with one macro or trait for each variant, is not 
an obvious stylistic improvement to me. Quite the opposite in fact.

> As I hinted in the RFR email, I tried to have one EnableIf-style API that
> handles an argument that can be either a trait type or a constexpr bool
> value. Something like
>
>    template<typename T, ENABLE_IF(std::is_integral<T>)>
>    void foo(T x) { ... }
>
>    template<typename T1, typename T2, ENABLE_IF(sizeof(T1) == sizeof(T2))>
>    void foo(T1 x, T2 y) { ... }
>
> Unfortunately, all my attempts at that tripped over MSVC bugs. And my bug
> report was closed as won't fix.
>
> (gcc and clang worked fine for the most part. One of the many variants I
> tried (while failing to get past MSVC) ICE'd gcc, which I reported and was
> accepted as an ice-on-valid-code regression.)

Yeah. I think that having ENABLE_IF accept both types and values would 
be less confusing compared to having EnableIfX vs EnableIfT. But I think 
the even less confusing solution is to have an ENABLE_IF that only 
accepts bools, and make that the convention explicitly on the call site. 
So your example:

   template<typename T, ENABLE_IF(std::is_integral<T>)>
   void foo(T x) { ... }

would just have to adapt to the value SFINAE convention and do:

   template<typename T, ENABLE_IF(std::is_integral<T>::value)>
   void foo(T x) { ... }

Suddenly there is no need for ENABLE_IF to support both booleans and 
types, and there is a clear convention how to do SFINAE: with values.

If the argument is that it is not pretty to have to write "::value", 
then I think the same stylistic comment can be said about having to 
append "T" to "EnableIf" to declare that you have this or that type of 
EnableIf that either will do the "::value" conversion for you internally 
or not.

>> So basically, my feedback is that I think you can safely remove EnableIfT and rename EnableIfX to EnableIf instead.
> Not while we still have (currently ~100) uses of our (perhaps legacy)
> EnableIf, which is significantly different. I'm not interested in developing
> or reviewing a change of all the existing EnableIf for function templates to
> use the new additional template parameter mechanism in one changeset.

While I think having a legacy style that we are telling people not to 
use is unfortunate, I do understand that such a cleanup is a larger 
undertaking. So if the concern is that the identifier "EnableIf" is 
already taken today, then I think you can do what your previous example 
did: call the macro variant used inside of the template arguments 
ENABLE_IF instead, and have it take values only. Then we can transition 
away from using now "legacy" EnableIf gradually. And seeing as it is a 
macro, using capital letters is the natural name anyway.

Hope this all makes sense.

Thanks,
/Erik