[PATCH] Default initialize enum flags to 0

[PATCH] Default initialize enum flags to 0

[Simon Marchi]

... so that we don't need to do it manually, and potentially forget.
For example, this allows to do:

  my_flags flags;

  ...

  flags |= some_flag;

gdb/ChangeLog:

	* common/enum-flags.h (enum_flags::enum_flags): Initialize
	m_enum_value to 0 in default constructor.
---
 gdb/common/enum-flags.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/gdb/common/enum-flags.h b/gdb/common/enum-flags.h
index 5b8c3ebc32..b1cf9a4644 100644
--- a/gdb/common/enum-flags.h
+++ b/gdb/common/enum-flags.h
@@ -117,6 +117,7 @@ private:
 public:
   /* Allow default construction, just like raw enums.  */
   enum_flags ()
+    : m_enum_value ((enum_type) 0)
   {}
 
   enum_flags (const enum_flags &other)
-- 
2.11.0

Re: [PATCH] Default initialize enum flags to 0

[Pedro Alves]

On 02/20/2017 09:45 PM, Simon Marchi wrote:
> ... so that we don't need to do it manually, and potentially forget.
> For example, this allows to do:
> 
>   my_flags flags;
> 
>   ...
> 
>   flags |= some_flag;
> 
> gdb/ChangeLog:
> 
> 	* common/enum-flags.h (enum_flags::enum_flags): Initialize
> 	m_enum_value to 0 in default constructor.

Not sure I really like this.  3 reasons off hand.  None are
very strong, but let me put them out nonetheless:

#1 - I have some desire of creating a "gdb/gnu template library" dir
and moving these utilities there, in order to share them with more
projects (e.g., gcc, and who knows, gold and who knows other parts
of binutils that might want to convert to C++ in the future), and
it'd be nice to keep the type behaving the same in C and C++
modes (that's why I had left the !__cplusplus branch in
the file).  [1].

#2 - The other reason is that it's nice IMO to leave enums and enum flags
easily interchangeable -- i.e., make them behave as close as possible.
Having one be default initialized, and the other value initialized
means that when changing variables from one type to the other
one needs to consider that aspect.

#3 - Default initializing to zero can hide bugs that would otherwise
be caught with -Winitialized.

[1] - Trevor expressed something similar before too [2]:
https://sourceware.org/ml/gdb-patches/2016-11/msg00114.html

[2] - Yes, I need to find some time to post a v2 of that
      series.  :-P  (It's baking slowing on my github.)

Thanks,
Pedro Alves

Re: [PATCH] Default initialize enum flags to 0

[Simon Marchi]

On 2017-02-20 18:18, Pedro Alves wrote:
> On 02/20/2017 09:45 PM, Simon Marchi wrote:
>> ... so that we don't need to do it manually, and potentially forget.
>> For example, this allows to do:
>> 
>>   my_flags flags;
>> 
>>   ...
>> 
>>   flags |= some_flag;
>> 
>> gdb/ChangeLog:
>> 
>> 	* common/enum-flags.h (enum_flags::enum_flags): Initialize
>> 	m_enum_value to 0 in default constructor.
> 
> Not sure I really like this.  3 reasons off hand.  None are
> very strong, but let me put them out nonetheless:
> 
> #1 - I have some desire of creating a "gdb/gnu template library" dir
> and moving these utilities there, in order to share them with more
> projects (e.g., gcc, and who knows, gold and who knows other parts
> of binutils that might want to convert to C++ in the future), and
> it'd be nice to keep the type behaving the same in C and C++
> modes (that's why I had left the !__cplusplus branch in
> the file).  [1].

I had the intuition that trying to keep the same behavior as plain C 
enums was a reason the field is currently left uninitialized.

> #2 - The other reason is that it's nice IMO to leave enums and enum 
> flags
> easily interchangeable -- i.e., make them behave as close as possible.
> Having one be default initialized, and the other value initialized
> means that when changing variables from one type to the other
> one needs to consider that aspect.

Well, they're not directly interchangeable in C++, which is the whole 
point of having enum flags.  But let's assume that they are for the sake 
of argument, and that we are initializing the value to 0 in the default 
constructor.  If you want to switch from enums to enum flags, the 
explicit zero-initializations you have in your code will now be 
extraneous but harmless.  If you are going from enum flags to enums you 
might be missing some initializations, but -Wuninitialized will tell 
you.  If you decide to use ints with #defines instead, then 
-Wuninitialized will tell you as well.

If you are switching back and forth between enums and enum flags in a C 
program, -Wuninitialized should warn you either way, and you'll have the 
same bug in both versions (since the enum flags type is a direct typedef 
to the enum type).

If the context is a program that is both C and C++, like GDB was not so 
long ago, then omitting an initialization will not be a bug in C++.  It 
will be a bug in C, but then again -Wuninitialized will warn you.

> #3 - Default initializing to zero can hide bugs that would otherwise
> be caught with -Winitialized.

(-Wuninitialized?)

I don't really understand how this could hide a bug.  When we don't 
initialize the field in the default constructor, does -Wuninitialized 
issue a warning for this?

   my_flags flags;
   flags |= some_flag;

I tried quickly and it doesn't seem so.  As stated above, if we have the 
default constructor of the enum flag initialize the value to 0, it won't 
be a bug in C++, but it will generate a warning in C where plain enums 
are used.

So if we don't initialize the value to 0 in the default constructor, 
compiling this code in C++ will be a bug but will not generate any 
warning.  This seems very error prone to me.

Simon

Re: [PATCH] Default initialize enum flags to 0

[Pedro Alves]

On 02/21/2017 03:01 AM, Simon Marchi wrote:

> 
>> #2 - The other reason is that it's nice IMO to leave enums and enum flags
>> easily interchangeable -- i.e., make them behave as close as possible.
>> Having one be default initialized, and the other value initialized
>> means that when changing variables from one type to the other
>> one needs to consider that aspect.
> 
> Well, they're not directly interchangeable in C++, which is the whole
> point of having enum flags.  

TBC, by "interchangeable" I meant, when you refactor/redesign code and 
decide the flags would be better as normal enums, and vice versa.

Passing an enum flags to a function expecting a raw enum
(because it was compiled in C) and vice versa would probably
not be interchangeable at run time, depending on ABI.

>> #3 - Default initializing to zero can hide bugs that would otherwise
>> be caught with -Winitialized.
> 
> (-Wuninitialized?)
> 
> I don't really understand how this could hide a bug.  

I was thinking of the "this code path should have set flags to something
non-zero, but the compiler didn't warn because the variable
was initialized" kind of bug.

> When we don't
> initialize the field in the default constructor, does -Wuninitialized
> issue a warning for this?
> 
>   my_flags flags;
>   flags |= some_flag;
> 
> I tried quickly and it doesn't seem so.  As stated above, if we have the
> default constructor of the enum flag initialize the value to 0, it won't
> be a bug in C++, but it will generate a warning in C where plain enums
> are used.

Bah, I assumed it did!  But now that I try, it really doesn't.  :-(

I filed a GCC bug now:

 [-Wuninitialized] referencing uninitialized field of POD struct should warn
 https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79658

This was my strongest argument, and I'm left without it, so...

> So if we don't initialize the value to 0 in the default constructor,
> compiling this code in C++ will be a bug but will not generate any
> warning.  This seems very error prone to me.

Agreed, unfortunately...

Looking at the patch:

> @@ -117,6 +117,7 @@ private:
>  public:
>    /* Allow default construction, just like raw enums.  */
>    enum_flags ()
> +    : m_enum_value ((enum_type) 0)
>    {}
>  

The "just like raw enums" comment is no longer true.  Please tweak that.

OK with that fixed.

Thanks,
Pedro Alves

Re: [PATCH] Default initialize enum flags to 0

[Simon Marchi]

On 2017-02-21 06:16, Pedro Alves wrote:
> On 02/21/2017 03:01 AM, Simon Marchi wrote:
> 
>> 
>>> #2 - The other reason is that it's nice IMO to leave enums and enum 
>>> flags
>>> easily interchangeable -- i.e., make them behave as close as 
>>> possible.
>>> Having one be default initialized, and the other value initialized
>>> means that when changing variables from one type to the other
>>> one needs to consider that aspect.
>> 
>> Well, they're not directly interchangeable in C++, which is the whole
>> point of having enum flags.
> 
> TBC, by "interchangeable" I meant, when you refactor/redesign code and
> decide the flags would be better as normal enums, and vice versa.
> 
> Passing an enum flags to a function expecting a raw enum
> (because it was compiled in C) and vice versa would probably
> not be interchangeable at run time, depending on ABI.
> 
>>> #3 - Default initializing to zero can hide bugs that would otherwise
>>> be caught with -Winitialized.
>> 
>> (-Wuninitialized?)
>> 
>> I don't really understand how this could hide a bug.
> 
> I was thinking of the "this code path should have set flags to 
> something
> non-zero, but the compiler didn't warn because the variable
> was initialized" kind of bug.
> 
>> When we don't
>> initialize the field in the default constructor, does -Wuninitialized
>> issue a warning for this?
>> 
>>   my_flags flags;
>>   flags |= some_flag;
>> 
>> I tried quickly and it doesn't seem so.  As stated above, if we have 
>> the
>> default constructor of the enum flag initialize the value to 0, it 
>> won't
>> be a bug in C++, but it will generate a warning in C where plain enums
>> are used.
> 
> Bah, I assumed it did!  But now that I try, it really doesn't.  :-(
> 
> I filed a GCC bug now:
> 
>  [-Wuninitialized] referencing uninitialized field of POD struct should 
> warn
>  https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79658

Oh so it's only in some very specific cases that the warning is 
missing...

> This was my strongest argument, and I'm left without it, so...

TBH, my original motivation was so we could be lazy and leave out the 
initializations, I only realized while reading your message that it was 
actually a problem :).

>> So if we don't initialize the value to 0 in the default constructor,
>> compiling this code in C++ will be a bug but will not generate any
>> warning.  This seems very error prone to me.
> 
> Agreed, unfortunately...
> 
> Looking at the patch:
> 
>> @@ -117,6 +117,7 @@ private:
>>  public:
>>    /* Allow default construction, just like raw enums.  */
>>    enum_flags ()
>> +    : m_enum_value ((enum_type) 0)
>>    {}
>> 
> 
> The "just like raw enums" comment is no longer true.  Please tweak 
> that.
> 
> OK with that fixed.

Thanks, pushed with that fixed.