Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix implementation of gsl_REQUIRES_T_() #238

Closed
wants to merge 3 commits into from
Closed

Fix implementation of gsl_REQUIRES_T_() #238

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0733f31
Change requires to use a default template argument (nonstd-lite-proje…
martinmoene Mar 6, 2020
9bc6e16
[skip ci] Remove comma from comment
martinmoene Mar 6, 2020
015a2a4
For MSVC 12.0 (VS2013, _MSC_VER:1800) fall back on previous behavior
martinmoene Mar 6, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 9 additions & 5 deletions include/gsl/gsl-lite.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -603,7 +603,7 @@
// For functions, prefer return-type SFINAE if possible.
// If return-type SFINAE is not applicable, use `gsl_REQUIRES_A_()` or `typename std::enable_if< VA, int >::type = 0` in the function template argument list.
//
// Use `gsl_REQUIRES_T_()` or `typename = typename std::enable_if< VA, ::gsl::detail::enabler >::type` in class template argument lists.
// Use `gsl_REQUIRES_T_()` or `typename std::enable_if< (VA), int >::type = 0` in class template argument lists.

#if gsl_HAVE( EXPRESSION_SFINAE )
# define gsl_DECLTYPE_(T, EXPR) decltype( EXPR )
Expand All @@ -612,7 +612,12 @@
#endif

#if gsl_HAVE( TYPE_TRAITS )
# define gsl_REQUIRES_T_(VA) , typename = typename std::enable_if< ( VA ), ::gsl::detail::enabler >::type
# if gsl_BETWEEN( gsl_COMPILER_MSVC_VERSION, 1, 140 )
// VS 2013 and earlier seem to have trouble with SFINAE for default non-type arguments
# define gsl_REQUIRES_T_(VA) , typename = typename std::enable_if< ( VA ), ::gsl::detail::enabler >::type
# else
# define gsl_REQUIRES_T_(VA) , typename std::enable_if< ( VA ), int >::type = 0
# endif
Comment on lines +615 to +620
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

#else
# define gsl_REQUIRES_T_(VA)
#endif
Expand Down Expand Up @@ -1085,13 +1090,12 @@ struct is_compatible_container : std17::bool_constant

template<
class C, class E
, typename = typename std::enable_if<
gsl_REQUIRES_T_((
Copy link
Collaborator

@mbeutel mbeutel Mar 6, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commenting at the toplevel.

! is_span< C >::value
&& ! is_array< C >::value
&& ! is_std_array< C >::value
&& ( std::is_convertible< typename std::remove_pointer<decltype( std17::data( std::declval<C&>() ) )>::type(*)[], E(*)[] >::value)
// && has_size_and_data< C >::value
, enabler>::type
))
, class = decltype( std17::size(std::declval<C>()) )
, class = decltype( std17::data(std::declval<C>()) )
>
Expand Down