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(-)

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))
>   	    {
>

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))
>>           {
>>
>

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<ovl_op_code>::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.

There's an updated version of the patch, Jonathan noticed correctly
the comment related to assert was not correct.

Martin

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<ovl_op_code>::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

On Thu, 22 Apr 2021 at 15:59, Martin Sebor <msebor@gmail.com> 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<ovl_op_code>::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.

On 4/22/21 9:41 AM, Jonathan Wakely wrote:
> On Thu, 22 Apr 2021 at 15:59, Martin Sebor <msebor@gmail.com> 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<ovl_op_code>::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

On Thu, 22 Apr 2021 14:28:24 -0600
Martin Sebor via Gcc-patches <gcc-patches@gcc.gnu.org> 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.

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