Re: [PATCH] Default initialize enum flags to 0

[Pedro Alves <palves@redhat.com>]

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