[llvm] r226490 - IR: Remove direct comparisons against Metadata::Storage, NFC

[Duncan P. N. Exon Smith]

> On 2015 Jan 20, at 10:14, David Blaikie <dblaikie at gmail.com> wrote:
> 
> 
> 
> On Tue, Jan 20, 2015 at 9:07 AM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
> 
> > On 2015 Jan 20, at 08:38, Timur Iskhodzhanov <timurrrr at google.com> wrote:
> >
> > On Tue Jan 20 2015 at 6:58:52 PM Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
> > > On 2015 Jan 20, at 07:53, Timur Iskhodzhanov <timurrrr at google.com> wrote:
> > >
> > > This is what I get on my Windows machine:
> > >   $ cat test.cpp
> > >   char hello;
> > >   $ bin\clang.exe -cc1 -gline-tables-only -triple i686-pc-linux -emit-obj -o z test.cpp
> > >   Assertion failed: DbgNode->isTemporary() && "Expected temporary node", file ..\lib\IR\DebugInfo.cpp, line 350
> > > but on Linux it's fine.
> > >
> > > Replacing
> > >   StorageType Storage : 2;
> > > with
> > >   unsigned Storage : 2;
> > > seems to resolve the issue.
> > > Shall I commit this change?
> > >
> >
> > Thanks for tracking this down!
> >
> > You've just won a black belt in remote debugging :)
> >
> > I guessed at the same thing and got someone on IRC to confirm.  Already
> > committed in r226570.
> >
> > Thanks, the Chromium bots are cycling green now!
> >
> > > Reid might know the enum stuff better.
> >
> > It's not an enum problem, as it turns out.  Same difference in behaviour
> > would happen with `int Storage : 2`.
> >
> > MSVC makes the top bit of the bitfield the sign bit; other compilers
> > "chop off" the sign bit.  It's implementation-defined which behaviour
> > you get (so MSVC isn't actually wrong here, just annoyingly different).
> >
> > I should have remembered this :/.  Sorry for the trouble.
> >
> > Do you think we could somehow find such bugs automatically?
> 
> If you had a bot running with your configuration sending emails and/or
> yelling at the IRC channel, I would have noticed pretty soon after my
> commit :).
> 
> > And/or update the developer policy?
> 
> Sure.  I think this sort of thing belongs in the coding standards [1],
> although I'm not quite sure where to put it.
> 
> [1]: http://llvm.org/docs/CodingStandards.html
> 
> Feel free to submit a patch!
> 
> BTW, Aaron Ballman (on IRC) pointed out that there's specific language in
> the standard to deal with enum bitfields:
> 
> >  [class.bit] calls out enumerations explicitly: "If the value of an enumerator is stored into a bit-field of the same enumeration type and the number of bits in the bit-field is large enough to hold all the values of that enumeration type (7.2), the original enumerator value and the value of the bit-field shall compare equal. "
> 
> So this probably *is* an MSVC bug.  Not that it matters too much; we need
> to avoid it either way.
> 
> Yeah, it's one of those bits of community lore that one should steer clear of enum bitfields because of MSVC and signed bitfields. I never remember the specifics of why they're bad, just that they are - agreed, it can/should be written down somewhere (people will still forget, but at least we'll have the explanation written somewhere, etc). Maybe the style guide needs an expanded bit of "things not to do" which would include the existing bit of which C++11 features we can use (links to compatibility matrices for supported compilers, maybe highlight common gotchas like MSVC's lack of compiler-generated move ops) this enum thing, etc.
>  

SGTM.  I'll try to circle back to this myself if no one beats me to it.