Re: [PATCH] Use STATIC_ASSERT for OVL_OP_MAX.
On 4/22/21 4:55 AM, Martin Liška wrote: There's an updated version of the patch, Jonathan noticed correctly the comment related to assert was not correct. Subject: [PATCH] Use STATIC_ASSERT for OVL_OP_MAX. Subject line needs "c++:" Please also include the rationale from your first message before the ChangeLog entries. OK with those adjustments. Jason
Re: [PATCH] Use STATIC_ASSERT for OVL_OP_MAX.
On Thu, 22 Apr 2021 14:28:24 -0600 Martin Sebor via Gcc-patches wrote: > > enum E { e = 5 }; > > struct A { E e: 3; }; > > > > constexpr int number_of_bits () > > { > > A a = { }; > > a.e = (E)-1; > > return 32 - __builtin_clz(a.e); > > } > > > > I had the same thought about using clz. It works in this case but > not in if one of the enumerators is negative, or if the underlying > type is signed. or for -fshort-enums in this case as is.
Re: [PATCH] Use STATIC_ASSERT for OVL_OP_MAX.
On 4/22/21 9:41 AM, Jonathan Wakely wrote: On Thu, 22 Apr 2021 at 15:59, Martin Sebor wrote: On 4/22/21 2:52 AM, Jonathan Wakely wrote: On Thu, 22 Apr 2021, 08:47 Martin Liška, wrote: On 4/21/21 6:11 PM, Martin Sebor wrote: > On 4/21/21 2:15 AM, Martin Liška wrote: >> Hello. >> >> It's addressing the following Clang warning: >> cp/lex.c:170:45: warning: result of comparison of constant 64 with expression of type 'enum ovl_op_code' is always true [-Wtautological-constant-out-of-range-compare] >> >> Patch can bootstrap on x86_64-linux-gnu and survives regression tests. >> >> Ready to be installed? >> Thanks, >> Martin >> >> gcc/cp/ChangeLog: >> >> * cp-tree.h (STATIC_ASSERT): Prefer static assert. >> * lex.c (init_operators): Remove run-time check. >> --- >> gcc/cp/cp-tree.h | 3 +++ >> gcc/cp/lex.c | 2 -- >> 2 files changed, 3 insertions(+), 2 deletions(-) >> >> diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h >> index 81ff375f8a5..a8f72448ea9 100644 >> --- a/gcc/cp/cp-tree.h >> +++ b/gcc/cp/cp-tree.h >> @@ -5916,6 +5916,9 @@ enum ovl_op_code { >> OVL_OP_MAX >> }; >> +/* Make sure it fits in lang_decl_fn::operator_code. */ >> +STATIC_ASSERT (OVL_OP_MAX < (1 << 6)); >> + > > I wonder if there's a way to test this directly by something like > > static_assert (number-of-bits (ovl_op_info_t::ovl_op_code) > <= number-of-bits (lang_decl_fn::operator_code)); Good point, but I'm not aware of it. Maybe C++ people can chime in? ovl_op_code is an unscoped enumeration (meaning "enum" not "enum class") with no fixed underlying type (i.e. no enum-base like ": int" or ": long" is specified) which means that the number of bits in is value representation is the number of bits needed to represent the minimum and maximum enumerators: "the values of the enumeration are the values representable by a hypothetical integer type with minimal width M such that all enumerators can be represented." There is no function/utility like number-of-bits that can tell you that from the type though.You could use std::underlying_type::type to get the integral type that the compiler used to represent it, but that will probably be 'int' in this case and so all it tells you is an upper bound of no more than 32 bits, which is not useful for this purpose. I suspected there wasn't a function like that. Thanks for confirming it. I wrote the one below just to see if it could be done. It works for one bit-field but I can't think of a way to generalize it. We'd probably need a built-in for that. Perhaps one might be useful. enum E { e = 5 }; struct A { E e: 3; }; constexpr int number_of_bits () { A a = { }; a.e = (E)-1; int n = 0; for (; a.e; ++n) a.e = (E)((unsigned)a.e ^ (1 << n)); return n; } Martin Or: enum E { e = 5 }; struct A { E e: 3; }; constexpr int number_of_bits () { A a = { }; a.e = (E)-1; return 32 - __builtin_clz(a.e); } I had the same thought about using clz. It works in this case but not in if one of the enumerators is negative, or if the underlying type is signed. But you can't get the number-of-bits needed for all the values of the enum E, which is what I was referring to. If you know the enumerators go from 0 to MAX (as is the case for ovl_op_code) you can use (32 - __builtin_clz(MAX)) there too, but in the general case you don't always know the maximum enumerator without checking, and it depends whether the enumeration has a fixed underlying type. That might be another useful query to add a built-in or trait for to improve introspection: get the min and max enumerator (or even all of them, e.g., as an initializer_list or something like that). Martin
Re: [PATCH] Use STATIC_ASSERT for OVL_OP_MAX.
On Thu, 22 Apr 2021 at 15:59, Martin Sebor wrote: > > On 4/22/21 2:52 AM, Jonathan Wakely wrote: > > On Thu, 22 Apr 2021, 08:47 Martin Liška, wrote: > > > > On 4/21/21 6:11 PM, Martin Sebor wrote: > > > On 4/21/21 2:15 AM, Martin Liška wrote: > > >> Hello. > > >> > > >> It's addressing the following Clang warning: > > >> cp/lex.c:170:45: warning: result of comparison of constant 64 > > with expression of type 'enum ovl_op_code' is always true > > [-Wtautological-constant-out-of-range-compare] > > >> > > >> Patch can bootstrap on x86_64-linux-gnu and survives regression > > tests. > > >> > > >> Ready to be installed? > > >> Thanks, > > >> Martin > > >> > > >> gcc/cp/ChangeLog: > > >> > > >> * cp-tree.h (STATIC_ASSERT): Prefer static assert. > > >> * lex.c (init_operators): Remove run-time check. > > >> --- > > >> gcc/cp/cp-tree.h | 3 +++ > > >> gcc/cp/lex.c | 2 -- > > >> 2 files changed, 3 insertions(+), 2 deletions(-) > > >> > > >> diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h > > >> index 81ff375f8a5..a8f72448ea9 100644 > > >> --- a/gcc/cp/cp-tree.h > > >> +++ b/gcc/cp/cp-tree.h > > >> @@ -5916,6 +5916,9 @@ enum ovl_op_code { > > >> OVL_OP_MAX > > >> }; > > >> +/* Make sure it fits in lang_decl_fn::operator_code. */ > > >> +STATIC_ASSERT (OVL_OP_MAX < (1 << 6)); > > >> + > > > > > > I wonder if there's a way to test this directly by something like > > > > > > static_assert (number-of-bits (ovl_op_info_t::ovl_op_code) > > > <= number-of-bits (lang_decl_fn::operator_code)); > > > > Good point, but I'm not aware of it. Maybe C++ people can chime in? > > > > > > ovl_op_code is an unscoped enumeration (meaning "enum" not "enum class") > > with no fixed underlying type (i.e. no enum-base like ": int" or ": > > long" is specified) which means that the number of bits in is value > > representation is the number of bits needed to represent the minimum and > > maximum enumerators: > > > > "the values of the enumeration are the values representable by a > > hypothetical integer type with minimal width M such that all enumerators > > can be represented." > > > > There is no function/utility like number-of-bits that can tell you that > > from the type though.You could use > > std::underlying_type::type to get the integral type that > > the compiler used to represent it, but that will probably be 'int' in > > this case and so all it tells you is an upper bound of no more than 32 > > bits, which is not useful for this purpose. > > I suspected there wasn't a function like that. Thanks for confirming > it. I wrote the one below just to see if it could be done. It works > for one bit-field but I can't think of a way to generalize it. We'd > probably need a built-in for that. Perhaps one might be useful. > > enum E { e = 5 }; > struct A { E e: 3; }; > > constexpr int number_of_bits () > { >A a = { }; >a.e = (E)-1; >int n = 0; >for (; a.e; ++n) > a.e = (E)((unsigned)a.e ^ (1 << n)); >return n; > } > > Martin Or: enum E { e = 5 }; struct A { E e: 3; }; constexpr int number_of_bits () { A a = { }; a.e = (E)-1; return 32 - __builtin_clz(a.e); } But you can't get the number-of-bits needed for all the values of the enum E, which is what I was referring to. If you know the enumerators go from 0 to MAX (as is the case for ovl_op_code) you can use (32 - __builtin_clz(MAX)) there too, but in the general case you don't always know the maximum enumerator without checking, and it depends whether the enumeration has a fixed underlying type.
Re: [PATCH] Use STATIC_ASSERT for OVL_OP_MAX.
On 4/22/21 2:52 AM, Jonathan Wakely wrote: On Thu, 22 Apr 2021, 08:47 Martin Liška, wrote: On 4/21/21 6:11 PM, Martin Sebor wrote: > On 4/21/21 2:15 AM, Martin Liška wrote: >> Hello. >> >> It's addressing the following Clang warning: >> cp/lex.c:170:45: warning: result of comparison of constant 64 with expression of type 'enum ovl_op_code' is always true [-Wtautological-constant-out-of-range-compare] >> >> Patch can bootstrap on x86_64-linux-gnu and survives regression tests. >> >> Ready to be installed? >> Thanks, >> Martin >> >> gcc/cp/ChangeLog: >> >> * cp-tree.h (STATIC_ASSERT): Prefer static assert. >> * lex.c (init_operators): Remove run-time check. >> --- >> gcc/cp/cp-tree.h | 3 +++ >> gcc/cp/lex.c | 2 -- >> 2 files changed, 3 insertions(+), 2 deletions(-) >> >> diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h >> index 81ff375f8a5..a8f72448ea9 100644 >> --- a/gcc/cp/cp-tree.h >> +++ b/gcc/cp/cp-tree.h >> @@ -5916,6 +5916,9 @@ enum ovl_op_code { >> OVL_OP_MAX >> }; >> +/* Make sure it fits in lang_decl_fn::operator_code. */ >> +STATIC_ASSERT (OVL_OP_MAX < (1 << 6)); >> + > > I wonder if there's a way to test this directly by something like > > static_assert (number-of-bits (ovl_op_info_t::ovl_op_code) > <= number-of-bits (lang_decl_fn::operator_code)); Good point, but I'm not aware of it. Maybe C++ people can chime in? ovl_op_code is an unscoped enumeration (meaning "enum" not "enum class") with no fixed underlying type (i.e. no enum-base like ": int" or ": long" is specified) which means that the number of bits in is value representation is the number of bits needed to represent the minimum and maximum enumerators: "the values of the enumeration are the values representable by a hypothetical integer type with minimal width M such that all enumerators can be represented." There is no function/utility like number-of-bits that can tell you that from the type though.You could use std::underlying_type::type to get the integral type that the compiler used to represent it, but that will probably be 'int' in this case and so all it tells you is an upper bound of no more than 32 bits, which is not useful for this purpose. I suspected there wasn't a function like that. Thanks for confirming it. I wrote the one below just to see if it could be done. It works for one bit-field but I can't think of a way to generalize it. We'd probably need a built-in for that. Perhaps one might be useful. enum E { e = 5 }; struct A { E e: 3; }; constexpr int number_of_bits () { A a = { }; a.e = (E)-1; int n = 0; for (; a.e; ++n) a.e = (E)((unsigned)a.e ^ (1 << n)); return n; } Martin
Re: [PATCH] Use STATIC_ASSERT for OVL_OP_MAX.
There's an updated version of the patch, Jonathan noticed correctly the comment related to assert was not correct. Martin >From e035fd0549ea17ab4f8d71488f577fd1e4077fd9 Mon Sep 17 00:00:00 2001 From: Martin Liska Date: Fri, 12 Mar 2021 14:32:07 +0100 Subject: [PATCH] Use STATIC_ASSERT for OVL_OP_MAX. gcc/cp/ChangeLog: * cp-tree.h (STATIC_ASSERT): Prefer static assert. * lex.c (init_operators): Remove run-time check. --- gcc/cp/cp-tree.h | 3 +++ gcc/cp/lex.c | 2 -- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h index 23a77a2b2e0..cb254e0adea 100644 --- a/gcc/cp/cp-tree.h +++ b/gcc/cp/cp-tree.h @@ -5922,6 +5922,9 @@ enum ovl_op_code { OVL_OP_MAX }; +/* Make sure it fits in lang_decl_fn::ovl_op_code. */ +STATIC_ASSERT (OVL_OP_MAX < (1 << 6)); + struct GTY(()) ovl_op_info_t { /* The IDENTIFIER_NODE for the operator. */ tree identifier; diff --git a/gcc/cp/lex.c b/gcc/cp/lex.c index 73e14b8394c..43abd019e6e 100644 --- a/gcc/cp/lex.c +++ b/gcc/cp/lex.c @@ -166,8 +166,6 @@ init_operators (void) if (op_ptr->name) { - /* Make sure it fits in lang_decl_fn::operator_code. */ - gcc_checking_assert (op_ptr->ovl_op_code < (1 << 6)); tree ident = set_operator_ident (op_ptr); if (unsigned index = IDENTIFIER_CP_INDEX (ident)) { -- 2.31.1
Re: [PATCH] Use STATIC_ASSERT for OVL_OP_MAX.
On Thu, 22 Apr 2021, 08:47 Martin Liška, wrote: > On 4/21/21 6:11 PM, Martin Sebor wrote: > > On 4/21/21 2:15 AM, Martin Liška wrote: > >> Hello. > >> > >> It's addressing the following Clang warning: > >> cp/lex.c:170:45: warning: result of comparison of constant 64 with > expression of type 'enum ovl_op_code' is always true > [-Wtautological-constant-out-of-range-compare] > >> > >> Patch can bootstrap on x86_64-linux-gnu and survives regression tests. > >> > >> Ready to be installed? > >> Thanks, > >> Martin > >> > >> gcc/cp/ChangeLog: > >> > >> * cp-tree.h (STATIC_ASSERT): Prefer static assert. > >> * lex.c (init_operators): Remove run-time check. > >> --- > >> gcc/cp/cp-tree.h | 3 +++ > >> gcc/cp/lex.c | 2 -- > >> 2 files changed, 3 insertions(+), 2 deletions(-) > >> > >> diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h > >> index 81ff375f8a5..a8f72448ea9 100644 > >> --- a/gcc/cp/cp-tree.h > >> +++ b/gcc/cp/cp-tree.h > >> @@ -5916,6 +5916,9 @@ enum ovl_op_code { > >> OVL_OP_MAX > >> }; > >> +/* Make sure it fits in lang_decl_fn::operator_code. */ > >> +STATIC_ASSERT (OVL_OP_MAX < (1 << 6)); > >> + > > > > I wonder if there's a way to test this directly by something like > > > > static_assert (number-of-bits (ovl_op_info_t::ovl_op_code) > > <= number-of-bits (lang_decl_fn::operator_code)); > > Good point, but I'm not aware of it. Maybe C++ people can chime in? > ovl_op_code is an unscoped enumeration (meaning "enum" not "enum class") with no fixed underlying type (i.e. no enum-base like ": int" or ": long" is specified) which means that the number of bits in is value representation is the number of bits needed to represent the minimum and maximum enumerators: "the values of the enumeration are the values representable by a hypothetical integer type with minimal width M such that all enumerators can be represented." There is no function/utility like number-of-bits that can tell you that from the type though.You could use std::underlying_type::type to get the integral type that the compiler used to represent it, but that will probably be 'int' in this case and so all it tells you is an upper bound of no more than 32 bits, which is not useful for this purpose.
Re: [PATCH] Use STATIC_ASSERT for OVL_OP_MAX.
On 4/21/21 6:11 PM, Martin Sebor wrote: > On 4/21/21 2:15 AM, Martin Liška wrote: >> Hello. >> >> It's addressing the following Clang warning: >> cp/lex.c:170:45: warning: result of comparison of constant 64 with >> expression of type 'enum ovl_op_code' is always true >> [-Wtautological-constant-out-of-range-compare] >> >> Patch can bootstrap on x86_64-linux-gnu and survives regression tests. >> >> Ready to be installed? >> Thanks, >> Martin >> >> gcc/cp/ChangeLog: >> >> * cp-tree.h (STATIC_ASSERT): Prefer static assert. >> * lex.c (init_operators): Remove run-time check. >> --- >> gcc/cp/cp-tree.h | 3 +++ >> gcc/cp/lex.c | 2 -- >> 2 files changed, 3 insertions(+), 2 deletions(-) >> >> diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h >> index 81ff375f8a5..a8f72448ea9 100644 >> --- a/gcc/cp/cp-tree.h >> +++ b/gcc/cp/cp-tree.h >> @@ -5916,6 +5916,9 @@ enum ovl_op_code { >> OVL_OP_MAX >> }; >> +/* Make sure it fits in lang_decl_fn::operator_code. */ >> +STATIC_ASSERT (OVL_OP_MAX < (1 << 6)); >> + > > I wonder if there's a way to test this directly by something like > > static_assert (number-of-bits (ovl_op_info_t::ovl_op_code) > <= number-of-bits (lang_decl_fn::operator_code)); Good point, but I'm not aware of it. Maybe C++ people can chime in? > > Also, since we are now compiling in C++ 11 mode, would using > static_assert be appropriate? We do: #if __cplusplus >= 201103L #define STATIC_ASSERT(X) \ static_assert ((X), #X) #else #define STATIC_ASSERT(X) \ typedef int assertion1[(X) ? 1 : -1] ATTRIBUTE_UNUSED #endif Btw. I'm going to send a patch with remove the #else branch. Martin > > Martin > > >> struct GTY(()) ovl_op_info_t { >> /* The IDENTIFIER_NODE for the operator. */ >> tree identifier; >> diff --git a/gcc/cp/lex.c b/gcc/cp/lex.c >> index 73e14b8394c..43abd019e6e 100644 >> --- a/gcc/cp/lex.c >> +++ b/gcc/cp/lex.c >> @@ -166,8 +166,6 @@ init_operators (void) >> if (op_ptr->name) >> { >> - /* Make sure it fits in lang_decl_fn::operator_code. */ >> - gcc_checking_assert (op_ptr->ovl_op_code < (1 << 6)); >> tree ident = set_operator_ident (op_ptr); >> if (unsigned index = IDENTIFIER_CP_INDEX (ident)) >> { >> >
Re: [PATCH] Use STATIC_ASSERT for OVL_OP_MAX.
On 4/21/21 2:15 AM, Martin Liška wrote: Hello. It's addressing the following Clang warning: cp/lex.c:170:45: warning: result of comparison of constant 64 with expression of type 'enum ovl_op_code' is always true [-Wtautological-constant-out-of-range-compare] Patch can bootstrap on x86_64-linux-gnu and survives regression tests. Ready to be installed? Thanks, Martin gcc/cp/ChangeLog: * cp-tree.h (STATIC_ASSERT): Prefer static assert. * lex.c (init_operators): Remove run-time check. --- gcc/cp/cp-tree.h | 3 +++ gcc/cp/lex.c | 2 -- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h index 81ff375f8a5..a8f72448ea9 100644 --- a/gcc/cp/cp-tree.h +++ b/gcc/cp/cp-tree.h @@ -5916,6 +5916,9 @@ enum ovl_op_code { OVL_OP_MAX }; +/* Make sure it fits in lang_decl_fn::operator_code. */ +STATIC_ASSERT (OVL_OP_MAX < (1 << 6)); + I wonder if there's a way to test this directly by something like static_assert (number-of-bits (ovl_op_info_t::ovl_op_code) <= number-of-bits (lang_decl_fn::operator_code)); Also, since we are now compiling in C++ 11 mode, would using static_assert be appropriate? Martin struct GTY(()) ovl_op_info_t { /* The IDENTIFIER_NODE for the operator. */ tree identifier; diff --git a/gcc/cp/lex.c b/gcc/cp/lex.c index 73e14b8394c..43abd019e6e 100644 --- a/gcc/cp/lex.c +++ b/gcc/cp/lex.c @@ -166,8 +166,6 @@ init_operators (void) if (op_ptr->name) { - /* Make sure it fits in lang_decl_fn::operator_code. */ - gcc_checking_assert (op_ptr->ovl_op_code < (1 << 6)); tree ident = set_operator_ident (op_ptr); if (unsigned index = IDENTIFIER_CP_INDEX (ident)) {
[PATCH] Use STATIC_ASSERT for OVL_OP_MAX.
Hello. It's addressing the following Clang warning: cp/lex.c:170:45: warning: result of comparison of constant 64 with expression of type 'enum ovl_op_code' is always true [-Wtautological-constant-out-of-range-compare] Patch can bootstrap on x86_64-linux-gnu and survives regression tests. Ready to be installed? Thanks, Martin gcc/cp/ChangeLog: * cp-tree.h (STATIC_ASSERT): Prefer static assert. * lex.c (init_operators): Remove run-time check. --- gcc/cp/cp-tree.h | 3 +++ gcc/cp/lex.c | 2 -- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h index 81ff375f8a5..a8f72448ea9 100644 --- a/gcc/cp/cp-tree.h +++ b/gcc/cp/cp-tree.h @@ -5916,6 +5916,9 @@ enum ovl_op_code { OVL_OP_MAX }; +/* Make sure it fits in lang_decl_fn::operator_code. */ +STATIC_ASSERT (OVL_OP_MAX < (1 << 6)); + struct GTY(()) ovl_op_info_t { /* The IDENTIFIER_NODE for the operator. */ tree identifier; diff --git a/gcc/cp/lex.c b/gcc/cp/lex.c index 73e14b8394c..43abd019e6e 100644 --- a/gcc/cp/lex.c +++ b/gcc/cp/lex.c @@ -166,8 +166,6 @@ init_operators (void) if (op_ptr->name) { - /* Make sure it fits in lang_decl_fn::operator_code. */ - gcc_checking_assert (op_ptr->ovl_op_code < (1 << 6)); tree ident = set_operator_ident (op_ptr); if (unsigned index = IDENTIFIER_CP_INDEX (ident)) { -- 2.31.1