`<type_traits>`: Implement `is_scoped_enum` #1950

SuperWig · 2021-06-02T10:35:30Z

I was looking for a place to put the new tests and got a bit lost in it all... so I think I need a bit of guidance there.

fixes: #1449

miscco

I think we need to reconsider the implementation strategy. From what I can see it should be much simpler

stl/inc/type_traits

miscco · 2021-06-02T10:47:18Z

stl/inc/type_traits

+inline constexpr bool is_scoped_enum_v<_Ty, false> = false;
+
+template <class _Ty>
+struct is_scoped_enum : bool_constant<is_scoped_enum_v<_Ty>> {};


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?

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?

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?

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.

stl/inc/type_traits

miscco · 2021-06-02T10:48:35Z

tests/std/tests/VSO_0000000_type_traits/test.cpp

@@ -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>);


I am missing some positive tests

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

stl/inc/type_traits

* Simplify is_scoped_enum_v thanks to miscco. * Add missing yvals_core.h macro comment. * Format with clang-format 11.

CaseyCarter · 2021-06-02T17:53:29Z

I was looking for a place to put the new tests and got a bit lost in it all... so I think I need a bit of guidance there.

I suggest tests\std\tests\VSO_0000000_type_traits\test.cpp.

SuperWig · 2021-06-02T18:08:16Z

Was kinda hoping for something a little more specific 😅 so I'll take that as "anywhere" then.

Going with just after these enums

STL/tests/std/tests/VSO_0000000_type_traits/test.cpp

Lines 856 to 860 in 696c536

    
           enum ExampleEnum { xExample, yExample }; 
        
           enum LLEnum : long long { xLongExample, yLongExample }; 
        
           enum class ExampleEnumClass { xExample, yExample }; 
        
           enum class LLEnumClass : long long { xLongExample, yLongExample };

And am I correct in thinking that these other tests don't need the new addition?

(probably would have been helpful of me to have included these questions in the OP...)

SuperWig · 2021-06-02T20:02:28Z

tests/std/tests/VSO_0000000_type_traits/test.cpp

@@ -851,6 +858,13 @@ 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);


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?

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.

StephanTLavavej · 2021-06-03T07:46:10Z

And am I correct in thinking that these other tests don't need the new addition?

I'd be fine with not updating the old tests. (There's a weak argument for updating them so they keep mentioning all type traits, but for example we stopped adding constexpr coverage to Dev11_1074023_constexpr and it was fine.)

As long as the new tests cover the "interesting" cases (e.g. void is always interesting - and something like a class type that can't be tested for convertibility), I think we'll be good to go.

stl/inc/yvals_core.h

StephanTLavavej · 2021-06-03T07:52:06Z

tests/std/tests/VSO_0000000_type_traits/test.cpp

@@ -851,6 +858,13 @@ 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);


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.

tests/std/tests/VSO_0000000_type_traits/test.cpp

StephanTLavavej · 2021-06-03T08:00:23Z

stl/inc/type_traits

+inline constexpr bool is_scoped_enum_v<_Ty, false> = false;
+
+template <class _Ty>
+struct is_scoped_enum : bool_constant<is_scoped_enum_v<_Ty>> {};


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.

Co-authored-by: Stephan T. Lavavej <[email protected]>

timsong-cpp · 2021-06-04T01:22:00Z

stl/inc/type_traits

+#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>>>;


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

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.

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?

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.

Ah, that makes sense. Thanks.

That second example is quite the eyebrow raiser...

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.

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.

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.

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

CaseyCarter

I'd like one more test case, otherwise this LGTM.

tests/std/tests/VSO_0000000_type_traits/test.cpp

StephanTLavavej · 2021-06-11T01:03:41Z

There's a libcxx test for is_scoped_enum but it won't run yet because we aren't mapping /std:c++latest to c++2b. I'll file a followup issue for that.

StephanTLavavej · 2021-06-12T02:49:39Z

Every time a PR merges, it takes us one step closer to the vision: A type trait on every desk and in every home. Thank you for your lasting contribution to STL history! 😺

Implement is_scoped_enum

36b02c6

SuperWig changed the title ~~Implement is_scoped_enum~~ <type_traits>: Implement is_scoped_enum Jun 2, 2021

miscco reviewed Jun 2, 2021

View reviewed changes

Small changes

696c536

* Simplify is_scoped_enum_v thanks to miscco. * Add missing yvals_core.h macro comment. * Format with clang-format 11.

CaseyCarter added the cxx23 C++23 feature label Jun 2, 2021

Add some test cases.

62c430f

SuperWig commented Jun 2, 2021

View reviewed changes

StephanTLavavej reviewed Jun 3, 2021

View reviewed changes

SuperWig and others added 2 commits June 3, 2021 09:33

Add space in comment

a18b27e

Co-authored-by: Stephan T. Lavavej <[email protected]>

One more test case.

12f939a

SuperWig marked this pull request as ready for review June 3, 2021 08:38

SuperWig requested a review from a team as a code owner June 3, 2021 08:38

timsong-cpp reviewed Jun 4, 2021

View reviewed changes

CaseyCarter self-assigned this Jun 9, 2021

CaseyCarter requested changes Jun 9, 2021

View reviewed changes

tests/std/tests/VSO_0000000_type_traits/test.cpp Show resolved Hide resolved

Add a test for edge case.

834c677

CaseyCarter approved these changes Jun 10, 2021

View reviewed changes

CaseyCarter removed their assignment Jun 10, 2021

Remove unnecessary qualification.

f95529a

StephanTLavavej reviewed Jun 11, 2021

View reviewed changes

tests/std/tests/VSO_0000000_type_traits/test.cpp Outdated Show resolved Hide resolved

StephanTLavavej approved these changes Jun 11, 2021

View reviewed changes

StephanTLavavej mentioned this pull request Jun 11, 2021

tests.py should map /std:c++latest to 'c++2b' #2001

Closed

StephanTLavavej self-assigned this Jun 11, 2021

StephanTLavavej merged commit 1ece8a0 into microsoft:main Jun 12, 2021

SuperWig deleted the scoped_enum branch June 12, 2021 04:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`<type_traits>`: Implement `is_scoped_enum` #1950

`<type_traits>`: Implement `is_scoped_enum` #1950

SuperWig commented Jun 2, 2021 •

edited

Loading

miscco left a comment

miscco Jun 2, 2021 •

edited

Loading

SuperWig Jun 2, 2021

SuperWig Jun 2, 2021

StephanTLavavej Jun 3, 2021

miscco Jun 2, 2021

SuperWig Jun 2, 2021

CaseyCarter commented Jun 2, 2021 •

edited

Loading

SuperWig commented Jun 2, 2021

SuperWig Jun 2, 2021 •

edited

Loading

StephanTLavavej Jun 3, 2021

StephanTLavavej commented Jun 3, 2021

StephanTLavavej Jun 3, 2021

StephanTLavavej Jun 3, 2021

timsong-cpp Jun 4, 2021

StephanTLavavej Jun 4, 2021

SuperWig Jun 4, 2021

cpplearner Jun 4, 2021 •

edited

Loading

SuperWig Jun 4, 2021

SuperWig Jun 4, 2021

StephanTLavavej Jun 4, 2021

CaseyCarter Jun 5, 2021 •

edited

Loading

frederick-vs-ja Mar 19, 2023 •

edited

Loading

cpplearner Mar 19, 2023

CaseyCarter left a comment

StephanTLavavej commented Jun 11, 2021

StephanTLavavej commented Jun 12, 2021

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

<type_traits>: Implement is_scoped_enum #1950

<type_traits>: Implement is_scoped_enum #1950

Conversation

SuperWig commented Jun 2, 2021 • edited Loading

miscco left a comment

Choose a reason for hiding this comment

miscco Jun 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CaseyCarter commented Jun 2, 2021 • edited Loading

SuperWig commented Jun 2, 2021

SuperWig Jun 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StephanTLavavej commented Jun 3, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpplearner Jun 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CaseyCarter Jun 5, 2021 • edited Loading

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CaseyCarter left a comment

Choose a reason for hiding this comment

StephanTLavavej commented Jun 11, 2021

StephanTLavavej commented Jun 12, 2021

`<type_traits>`: Implement `is_scoped_enum` #1950

`<type_traits>`: Implement `is_scoped_enum` #1950

SuperWig commented Jun 2, 2021 •

edited

Loading

miscco Jun 2, 2021 •

edited

Loading

CaseyCarter commented Jun 2, 2021 •

edited

Loading

SuperWig Jun 2, 2021 •

edited

Loading

cpplearner Jun 4, 2021 •

edited

Loading

CaseyCarter Jun 5, 2021 •

edited

Loading

frederick-vs-ja Mar 19, 2023 •

edited

Loading