Fix implementation of gsl_REQUIRES_T_()

[martinmoene]

Fixes implementation of gsl_REQUIRES_T_(), see nonstd-lite-project issue 40 and enable_if on CppReference.

[martinmoene]

Note: Visual Studio 2013 (VC12) fails with warning C4346 dependent name is not a type

Update: changed to fall back to previous behavior for VS2013.

[mbeutel]

What is the reason for this change? I understand that we want to use typename std::enable_if< X, int >::type = 0 rather than typename = typename std::enable_if< X >::type in function templates, but we already have gsl_REQUIRES_A_() for that purpose.

If the latter form is not required anywhere, perhaps we should remove gsl_REQUIRES_T_() altogether?

[martinmoene]

Ah great, you awoke me :) I have generalized the note about function templates to include class templates :)

Thus there is no reason for this change left.

[mbeutel]

Note that SFINAE via default template type arguments is fine in class definitions. (The two subsequent arguments class = decltype( ... ) also use default template type arguments for SFINAE.)

Also, our SFINAE Guidelines say that the gsl_REQUIRES_*_() macros should be avoided "if language support can be assumed in the given context", which is the case here.

[martinmoene]

Commenting at the toplevel.

[mbeutel]

Cf. #197 (comment), #197 (comment) for previous discussion of SFINAE and gsl_REQUIRES_*() in gsl-lite.

[martinmoene]

Thanks for you remarks and pointing to the discussion & comments (which i had (mostly) seen, but forgotten about).

Given all what I read, it's perhaps best to 'just' close this PR :)

---

This PR results from a change I made in the other *-lite libraries on the premise, erroneously generalized to templated classes, that for sfinae in a template parameter list

typename std::enable_if< (expression), int >::type = 0

is to be preferred over

typename = typename std::enable_if< (expression), ::gsl::detail::enabler >::type

based on this note over at CppReference.

On naming

From what I recall:

• gsl_REQUIRES_A_(): sfinae on a funtion or method templated Argument type
• gsl_REQUIRES_T_(): sfinae on a class, funtion or method Templated parameter type
• gsl_REQUIRES_R_(): sfinae on a funtion or method Return type

In hindsight, more appropriate letters might perhaps have been choosen (and perhaps still can:).

It looks like gsl-lite contains several uses of gsl_REQUIRES_A_() where gsl_REQUIRES_T_() is expected from above.

Code over requires() clause:

I'm always a little surprised over a preference for literal code over a searchable, contract-like requires clause as advocated by the Guidelines for SFINAE in gsl-lite (a matter of taste, I assume :)

// The macros below are for conditional SFINAE, i.e. they apply SFINAE only if the necessary language support is available.
//  Don't use these macros if language support can be assumed in the given context.

[mbeutel]

On naming

From what I recall:

* gsl_REQUIRES_**A**_(): sfinae on a funtion or method **templated Argument type**

* gsl_REQUIRES_**T**_(): sfinae on a class, funtion or method **Templated parameter type**

* gsl_REQUIRES_**R**_(): sfinae on a funtion or method **Return type**

That makes sense. But as per the discussion I linked to, function argument SFINAE should normally not be used (this is needed only for constructors in pre-C++11 and for buggy compilers). I figured that gsl_REQUIRES_A_() can also mean "non-type argument SFINAE" and is hence applicable for both template argument lists and function argument lists.

It looks like gsl-lite contains several uses of gsl_REQUIRES_A_() where gsl_REQUIRES_T_() is expected from above.

According to your definition, yes. I admit I don't know off hand whether non-type argument SFINAE is actually wrong for class template definitions; ideally we could use this form everywhere. I remember having had trouble even with recent MSVC versions when I tried this though. I'll investigate.

I'm always a little surprised over a preference for literal code over a searchable, contract-like requires clause as advocated by the Guidelines for SFINAE in gsl-lite (a matter of taste, I assume :)

I'd argue enable_if is searchable too :) Apart from that, I see several issues with the macros:

• People reading gsl-lite source code presumably already know what enable_if does. They may guess what the macro is doing but they will have to look it up for details. The problem that made you issue this PR is a perfect example:

    template< typename U = T
        optional_REQUIRES_T( ... )>
    optional_constexpr14 optional( optional && other )

  A reader might guess that optional_REQUIRES_T() is SFINAE in disguise, but she will wonder, "does the macro definition properly use a non-type argument to avoid ambiguity?" Contrariwise, if the SFINAE is spelled out explicitly, the answer is obvious:

    template< typename U = T,
        typename std::enable_if< ..., int >::type = 0 >
    optional_constexpr14 optional( optional && other )

• the syntax of gsl_REQUIRES_[T|A]_() is without precedence in C++:

  template <class X gsl_REQUIRES_A_( sizeof( X > 1 ) ) >

  For a casual reader this looks like there's a comma separator missing, which may yield misleading assumptions ("perhaps this is a bug which goes undetected because gsl_REQUIRES_A_() expands to nothing by default?").

• I am (grudgingly) in favor of macros which reduce complexity. For example, gsl_constexpr keeps me from having to write

  #if gsl_HAVE( CONSTEXPR_11 )
  constexpr
  #endif
  void example( ) { ... }

  But gsl_REQUIRES_*() doesn't actually reduce complexity. It just introduces a slight syntactic abbreviation. (Actually, three abbreviations for three different flavors of SFINAE. And which macro refers to which flavor? I'll have to look it up. And I tend to forget these little details quickly, so I have to look it up every time.)

The last point is also why the current SFINAE guidelines request that gsl_REQUIRES_*() be avoided except if SFINAE may be conditional: in that case the macro saves me from having to guard my SFINAE statements with #ifdefs.

Eli Bendersky's article on SFINAE, which was a valuable resource for me, comes to a similar conclusion:

Admittedly, std::enable_if is clumsy, and even enable_if_t doesn't help much, though it's a bit less verbose. You still have to mix it into the declaration of a function in a way that often obscures the return type or an argument type. This is why some sources online suggest crafting more advanced versions that "get out of the way". Personally, I think this is the wrong tradeoff to make.

std::enable_if is a rarely used construct. So making it less verbose doesn't buy us much. On the other hand, making it more mysterious is detrimental, because every time we see it we have to think about how it works. The implementation shown here is fairly simple, and I'd keep it this way. Finally I'll note that the C++ standard library uses the verbose, "clumsy" version of std::enable_if without defining more complex versions. I think that's the right decision.

Edit: I acknowledge your point that the macro attempts to look like a "contract-like requires clause". But unlike contracts (which should be used basically everywhere once they are available), SFINAE is a sneaky technique with many issues (e.g. significant compile time cost, illegible error messages, hard to debug). In my opinion, SFINAE should be used only when needed (typically for overload resolution). I'm aware that some people like to imitate concepts with SFINAE (e.g. range-v3, which is also infamous for slow compile times [but which arguably needs concepts]). I just think this style shouldn't be encouraged.

[martinmoene]

Thanks for your extensive reply. I can lean to your conclusions as well.

On the aspect of clarity by refraining from using macro gsl_REQUIRES(expression), one thing that affects me is that I may observe the expression more easily when using the macro. Simplicity and the need for visual guidance (informing my somewhat unusual laying out) is of importance to me, likely because of how my brains work (or do not :)

I think we've reached closure :)

---

In my opinion, SFINAE should be used only when needed (typically for overload resolution). I'm aware that some people like to imitate concepts with SFINAE (e.g. range-v3, which is also infamous for slow compile times [but which arguably needs concepts]). I just think this style shouldn't be encouraged.

However in the specifications of std::expected, std::optional, std::variant, std::span restrictions on methods abound.

[mbeutel]

Thanks for your extensive reply.

I'm grateful you still care about that child you have let go :)

On the aspect of clarity by refraining from using macro gsl_REQUIRES(expression), one thing that affects me is that I may observe the expression more easily when using the macro.

Yes, and I guess I've gotten so accustomed to syntax overhead that I completely overlooked this aspect of the macro. On reviewing gsl-lite, I agree that the constraints written with the macro are much easier to grasp than those where enable_if<> is spelled out.

However in the specifications of std::expected, std::optional, std::variant, std::span restrictions on methods abound.

True, SFINAE is often needed when defining vocabulary types... So I guess as long as we make it clear that the macro is for internal use (which was my intent in adding the trailing _), these macros are fine.

Reassessing the uses of gsl_REQUIRES_*() and enable_if in gsl-lite leads me to the following observations:

• gsl_REQUIRES_R_() is not used at all.
• We have only one use of gsl_REQUIRES_T_(), in the definition of owner<>.
• gsl_REQUIRES_A_() is used for not_null_ic<>, span<> and basic_string_span<> constructors, for byte operators and to_* conversion functions, where fallback to default type template arguments is acceptable (no potentially ambiguous overloads).
• gsl_REQUIRES_A_() is also used for constrained container constructors in span<> and basic_string_span<>, where this fallback is not acceptable; but the constrained constructors are available only if no fallback is necessary (cf. https://github.com/gsl-lite/gsl-lite/blob/15ca8279154933e8d8226f336b2fe63a394a128c/include/gsl/gsl-lite.hpp#L640..L641), so this is fine.
• template type argument enable_if<> is used in is_compatible_container<> (for old compilers only)
• template non-type argument enable_if<> is used extensively in not_null<>; there are dedicated definitions for compilers which don't support default function template arguments

So perhaps we can simplify this a little:

• remove gsl_REQUIRES_R_()
• find out if gsl_REQUIRES_T_() is necessary for classes or alias templates, or if we can use non-type template argument SFINAE there too; if so, remove gsl_REQUIRES_T_()
• the legacy definition of is_compatible_container<> isn't a proper type trait anyway (it cannot be false_type); try to fix this so we don't need enable_if<> here
• rename gsl_REQUIRES_A_() → gsl_ENABLE_IF_()
• replace uses of enable_if<> with gsl_ENABLE_IF_(); add a comment where the NTTP form is crucial (i.e. no fallback allowed)

Then we'd have only a single macro flavor, its name would no longer allude to concepts, and searching for "enable_if" would also find the macro. And we would be consistent again by using gsl_ENABLE_IF_() everywhere.

What do you think?

[martinmoene]

@mbeutel , will react later

[martinmoene]

You do make an interesting suggestion to see if all constraints might be taken under the wings of a single macro like gsl_ENABLE_IF_(), so that the only purpose and effect of the macro becomes to reduce 'clutter'.

I think that would be a nice result. That said, if you prefer using plain std::enable_if<> over using it via a macro like gsl_ENABLE_IF_() I'm completely happy with that also :)