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

Jump to bottom

<type_traits>: Implement is_scoped_enum #1950

Merged
merged 7 commits into from
Jun 12, 2021
+51 −0
Merged

<type_traits>: Implement is_scoped_enum #1950

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
36b02c6
Implement is_scoped_enum
SuperWig Jun 2, 2021
696c536
Small changes
SuperWig Jun 2, 2021
62c430f
Add some test cases.
SuperWig Jun 2, 2021
a18b27e
Add space in comment
SuperWig Jun 3, 2021
12f939a
One more test case.
SuperWig Jun 3, 2021
834c677
Add a test for edge case.
SuperWig Jun 10, 2021
f95529a
Remove unnecessary qualification.
StephanTLavavej Jun 11, 2021
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
9 changes: 9 additions & 0 deletions stl/inc/type_traits
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -366,6 +366,15 @@ struct is_enum : bool_constant<__is_enum(_Ty)> {}; // determine whether _Ty is a
template <class _Ty>
_INLINE_VAR constexpr bool is_enum_v = __is_enum(_Ty);

#if _HAS_CXX23
// STRUCT TEMPLATE is_scoped_enum
template <class _Ty>
inline constexpr bool is_scoped_enum_v = conjunction_v<is_enum<_Ty>, negation<is_convertible<_Ty, int>>>;
Copy link
Contributor

Choose a reason for hiding this comment

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

enum E  {
   e = std::is_scoped_enum_v<E>
};
static_assert(!e);

Copy link
Member

Choose a reason for hiding this comment

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

That's a diabolical brilliant brilliantly diabolical example. I think this means we need a compiler builtin.

The Standardese is WG21-N4885 [dcl.enum]/6 "An enumeration whose underlying type is not fixed is an incomplete type until the closing } of its enum-specifier, at which point it becomes a complete type." and [meta.rqmts]/5 "Unless otherwise specified, an incomplete type may be used to instantiate a template specified in 20.15." There are no additional preconditions for is_scoped_enum, so the incomplete enum E is permitted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I'm a little confused. Is that not supposed to compile?

enum E  {
   e1 = std::is_scoped_enum_v<E>,
   e2 = std::is_enum_v<E>
};
static_assert(!e1);
static_assert(e2);

This compiles just fine. If is_scoped_enum_v isn't supposed to work here then why does is_enum_v?

Copy link
Contributor

@cpplearner cpplearner Jun 4, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

IIUC

enum E  {
   a = std::is_convertible_v<E, int>, // UB because E is incomplete
   e = std::is_scoped_enum_v<E> // should be ok; value is false
};

But I think it's valid to rely on the implementation-specific knowledge that std::is_convertible_v works in such case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, that makes sense. Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That second example is quite the eyebrow raiser...

Copy link
Member

Choose a reason for hiding this comment

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

Ok, so it sounds like (a) is_convertible_v is theoretically an issue here, but (b) in practice, MSVC's nonconformance allows this to compile and get the right answer. In any event, (c) this is an extreme corner case.

I think we can proceed with the current implementation then. If we discover that the issue is worse than previously thought, we can always request a compiler builtin and replace the implementation.

Copy link
Member

@CaseyCarter CaseyCarter Jun 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I think we can proceed with the current implementation then. If we discover that the issue is worse than previously thought, we can always request a compiler builtin and replace the implementation.

I'm inclined to request intrinsics now so the library can be prepared for MSVC's enumeration bugs to be fixed. When some compiler dev goes to audit what things break with the bug fixed, I'd rather we don't show up on that list.

In practice, this still looks like "go ahead and merge this implementation", the only difference would really be that we may update it to use intrinsics a bit sooner.

Copy link
Contributor

@frederick-vs-ja frederick-vs-ja Mar 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I think we can proceed with the current implementation then. If we discover that the issue is worse than previously thought, we can always request a compiler builtin and replace the implementation.

FWIW, CWG-2516 has been accepted as a DR, so a scoped enum type is always complete when its name is declared, and hence no intrinsic is needed for is_scoped_enum. But we may still have to replace the current implementation once MSVC's bug (or non-conforming extension) is fixed.

Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed that MSVC has implemented /Zc:enumTypes (Enable enum type deduction), but it seems that despite this option, MSVC still believes that enumeration types are always complete, and that the underlying type is int before the closing }.

#include <type_traits>

template<class T>
constexpr std::underlying_type_t<T> f() {
  T t;
  return 1;
}

enum E {
  a = f<E>(),  // should be ill-formed
  b = sizeof(E),  // should be ill-formed
  c = 0x1'0000'0000,
};

static_assert(b == sizeof(E)); // fires under /Zc:enumTypes


template <class _Ty>
struct is_scoped_enum : bool_constant<is_scoped_enum_v<_Ty>> {};
Copy link
Contributor

@miscco miscco Jun 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I am a bit torn, usually we always follow the convention to define the variable template and then derive the struct from it.

In this case we require the short circuiting from conjunction, so maybe the "best" implementation would be to do

Suggested change
struct is_scoped_enum : bool_constant<is_scoped_enum_v<_Ty>> {};
struct is_scoped_enum : conjunction<is_enum<_Ty>, negation<is_convertible<_Ty, int>>> {};

And then derive the variable template from that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was originally under is_unbounded_array as that's where is_scoped_enum is depicted in the standard, and that implementation doesn't do that. (It was moved as is_enum and is_convertible are defined below).

Is there a general rule about this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I figure most people would be using the suffix version and doing it the traditional way would mean an extra template instantiation, wouldn't it?

Copy link
Member

Choose a reason for hiding this comment

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

Our modern convention is to follow the Standard order when it's easy to do so - however, the type traits are fairly jumbled, partly due to history, partly due to their dependencies. I wouldn't worry about the ordering.

As for whether the struct or the variable template should be "fundamental" - it's the struct that's costly for throughput, whereas variable templates are cheap. The compiler has built-in optimizations for conjunction_v, negation, is_convertible, and I think is_enum too. So, the code should have good throughput as-is.

While it's possible to duplicate the implementation to improve the struct throughput slightly, as @miscco depicted, I don't think that's necessary here (because I don't think that this type trait will be used in a massive number of specializations - whereas the traits like is_convertible are highly used). Additionally, note that if we did duplicate the implementation, we would want to "smash" the base class to true_type or false_type - i.e. derive from conjunction<ARGS>::type (or bool_constant<conjunction_v<ARGS>>) and not conjunction<ARGS> directly. (If you search, you'll see a few occurrences of this throughput the STL.) This simplifies the actual inheritance.

#endif // _HAS_CXX23

// STRUCT TEMPLATE is_compound
template <class _Ty>
struct is_compound : bool_constant<!is_fundamental_v<_Ty>> {}; // determine whether _Ty is a compound type
Expand Down
8 changes: 8 additions & 0 deletions stl/inc/yvals_core.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,6 +255,9 @@
// P1831R1 Deprecating volatile In The Standard Library
// Other C++20 deprecation warnings

// _HAS_CXX23 directly controls:
// P1048R1 is_scoped_enum

// Parallel Algorithms Notes
// C++ allows an implementation to implement parallel algorithms as calls to the serial algorithms.
// This implementation parallelizes several common algorithm calls, but not all.
Expand Down Expand Up @@ -1335,6 +1338,11 @@
#define __cpp_lib_coroutine 201902L
#endif // __cpp_impl_coroutine

// C++23
#if _HAS_CXX23
#define __cpp_lib_is_scoped_enum 202011L
#endif // _HAS_CXX23

// EXPERIMENTAL
#define __cpp_lib_experimental_erase_if 201411L
#define __cpp_lib_experimental_filesystem 201406L
Expand Down
3 changes: 3 additions & 0 deletions tests/std/tests/VSO_0000000_instantiate_type_traits/test.compile.pass.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,9 @@ void type_traits_test_impl() {
TRAIT_V(is_bounded_array, T);
TRAIT_V(is_unbounded_array, T);
#endif // _HAS_CXX20
#if _HAS_CXX23
TRAIT_V(is_scoped_enum, T);
#endif // _HAS_CXX23
TRAIT_V(rank, T);
TRAIT_V(extent, T);
TRAIT_V(is_same, T, U); // from xtr1common
Expand Down
15 changes: 15 additions & 0 deletions tests/std/tests/VSO_0000000_type_traits/test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,9 @@ STATIC_ASSERT(!is_unsigned_v<void>);
STATIC_ASSERT(!is_bounded_array_v<void>);
STATIC_ASSERT(!is_unbounded_array_v<void>);
#endif // _HAS_CXX20
#if _HAS_CXX23
STATIC_ASSERT(!is_scoped_enum_v<void>);
#endif // _HAS_CXX23

STATIC_ASSERT(!is_constructible_v<void>);
STATIC_ASSERT(!is_default_constructible_v<void>);
Expand Down Expand Up @@ -629,6 +632,10 @@ void test_function_type() {
STATIC_ASSERT(!is_bounded_array_v<T>);
STATIC_ASSERT(!is_unbounded_array_v<T>);
#endif // _HAS_CXX20

#if _HAS_CXX23
STATIC_ASSERT(!is_scoped_enum_v<T>);
Copy link
Contributor

Choose a reason for hiding this comment

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

I am missing some positive tests

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hence the draft PR and text body. Didn't know where the best place to put them in.

#endif
}

template <typename T>
Expand Down Expand Up @@ -851,6 +858,14 @@ enum LLEnum : long long { xLongExample, yLongExample };
enum class ExampleEnumClass { xExample, yExample };
enum class LLEnumClass : long long { xLongExample, yLongExample };

#if _HAS_CXX23
STATIC_ASSERT(!is_scoped_enum<ExampleEnum>::value);
Copy link
Contributor Author

@SuperWig SuperWig Jun 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Should I remove this?
I'm not seeing the structs used much.

Also I kinda forgot about the test on line 80, should I just create some enums up top (or move these enums?) and move this test block?

Copy link
Member

Choose a reason for hiding this comment

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

I think it's good to have minimal coverage that the struct form works.

The location of this test block seems fine to me - the previous locations in this file are doing different things.

STATIC_ASSERT(!is_scoped_enum_v<ExampleEnum>);
STATIC_ASSERT(!is_scoped_enum_v<test_abc1>);
STATIC_ASSERT(is_scoped_enum_v<ExampleEnumClass>);
STATIC_ASSERT(is_scoped_enum_v<LLEnumClass>);
#endif // _HAS_CXX23
CaseyCarter marked this conversation as resolved.
Show resolved Hide resolved

// P0258R2 has_unique_object_representations
#if _HAS_CXX17
STATIC_ASSERT(!has_unique_object_representations_v<void>);
Expand Down
14 changes: 14 additions & 0 deletions tests/std/tests/VSO_0157762_feature_test_macros/test.compile.pass.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -955,6 +955,20 @@ STATIC_ASSERT(__cpp_lib_is_pointer_interconvertible == 201907L);
#endif
#endif

#if _HAS_CXX23
#ifndef __cpp_lib_is_scoped_enum
#error __cpp_lib_is_scoped_enum is not defined
#elif __cpp_lib_is_scoped_enum != 202011L
#error __cpp_lib_is_scoped_enum is not 202011L
#else
STATIC_ASSERT(__cpp_lib_is_scoped_enum == 202011L);
#endif
#else
#ifdef __cpp_lib_is_scoped_enum
#error __cpp_lib_is_scoped_enum is defined
#endif
#endif

#if _HAS_CXX17
#ifndef __cpp_lib_is_swappable
#error __cpp_lib_is_swappable is not defined
Expand Down