Re: [PATCH] Default initialize enum flags to 0

[Simon Marchi <simon.marchi@polymtl.ca>]

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.