[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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

LGTM (aside from some small nits that you can fix when landing).



================
Comment at: compiler-rt/test/ubsan/TestCases/Misc/enum.cpp:27
+  return ((int)e1 != -1) & ((int)e2 != -1) &
+         // CHECK: error: load of value <unknown>, which is not a valid value for type 'enum EBool'
+         ((int)e3 != -1) & ((int)e4 == 1) &
----------------
shafik wrote:
> aaron.ballman wrote:
> > erichkeane wrote:
> > > What does THIS come from?  What value is unknown?  Shouldn't the -1 be fine?
> > +1, I'm surprised by the `<unknown>` there, but also... neither `e1` nor `e2` are of type `enum EBool`!
> So it looks like clang knows that the only valid values for a bool enum is 0 or 1 and it will mask the value accordingly see godbolt for example using `argc` : https://godbolt.org/z/ceb9hPno9
> 
> So that would explain why the original test used a `unsigned char*` in order to prompt the diagnostic. 
> 
> Looking into the ubsan diagnostic it looks like it treats bool as an unknown value, separate from integer and float. It is not clear to me why it does this but fixing that feels outside the scope of this change since this was part of the original test.
Thanks for looking into it! I agree that fixing the ubsan diagnostic behavior is out of scope. That said, can you move the CHECK lines so that they come *after* the line of code being checked? I think that's what threw me for a loop regarding the `EBool` confusion I had.


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

https://reviews.llvm.org/D130301