[PATCH] D130301: [Clang] Fix how we set the NumPositiveBits on an E numDecl to cover the case of single enumerator with value zero or an empty enum

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

My concern with the 'num positive bits' is really that `enum EEmpty{};`, `enum EZero{ A =0 };`, and `enum ENeg {A = -1};` should all have 1 total bit of 'values'.  So as long as THAT is true, I would expect this to be ok.

I fear that sticking to the 'non-negative' definition of `NumPositiveBits` might cause some oddball annoyances when trying to add this rule as a side-effect.



================
Comment at: clang/lib/Sema/SemaDecl.cpp:18886
+          InitVal.getActiveBits() ? InitVal.getActiveBits() : 1;
+      NumPositiveBits = std::max(NumPositiveBits, ActiveBits);
+    } else
----------------
shafik wrote:
> erichkeane wrote:
> > What about:
> > 
> > `std::max({NumPositiveBits, ActiveBits, 1})` (which uses the init-list version of std::max).
> > 
> > Also, not completely following this: BUT we don't need a positive bit IF we have a negative bit, right?  So this is only necessary if BOTH NumPositiveBits and NumNegativeBits would have been 0?
> Good call on the `std::max` alternative, I completely forgot about it.
> 
> If we look at the description of `getNumPositiveBits()` in `Decl.h` it says:
> 
> > Returns the width in bits required to store all the non-negative enumerators of this enum.
> 
> So since `0` is non-negative we should update the positive bits. Make sense?
Hmm... ok then.  As long as my test below ends up with the right values (that is, a value range of -1, 0 in the 1 case, and 0, 1 in the other).


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130301/new/

https://reviews.llvm.org/D130301