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
