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

[David Majnemer]

Why not just give the enum an underlying type?

On Tue, Jan 20, 2015 at 10:25 AM, Duncan P. N. Exon Smith <
dexonsmith at apple.com> wrote:

>
> > 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.
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150120/597e98ec/attachment.html>