[cfe-commits] [Patch] -Wduplicate-enum which fixes PR6343

[Chandler Carruth]

On Tue, Aug 14, 2012 at 10:46 AM, David Blaikie <dblaikie at gmail.com> wrote:

> Alternatively, or in addition to (so you can use the other 62 bits
> you'll get in addition to the 2 bits you'd end up using for the
> tombstone & sentinel flags) you could just throw out any enum that
> eventually uses one of the reserved values (just like you currently
> throw out any enum with a > 64 bit representation) - though there's a
> time/space/value tradeoff (time to bit twiddle, space to store the
> extra flags/bits, value in terms of which enum types you can diagnose)
>

I think there will be a significant performance hit to using DenseMap with
a key that is a struct of a 64bit integer and two bits, or two bools. You
suddenly have padding in the struct, use way more memory for the keys, etc.

However, I don't think there is any real interest in diagnosing enums with
keys spanning the 64-bit space, but the tricky thing is this:

enum {
  kMinValue = INT64_MIN,
  kMaxValue = INT64_MIN
};

So I worry about reserving any one value of the key space.

We can already build a DenseMap with a key of an APInt I think, why not
just use that, and skip enums with more than N enumerators for some N?

(I actually don't think we need to bother with skipping. If you use a
DenseMap so that the operations are generally constant time, this warning
should scale in proportion to the size of the input code. If people give us
an enum with more than 4 billion enumerators...)


>
> On Tue, Aug 14, 2012 at 10:33 AM, Ted Kremenek <kremenek at apple.com> wrote:
> > I can see a trivial workaround by sacrificing a bit of storage.  Wrap
> > the int64_t in a struct, with extra bits to indicate whether it is a
> > sentinel or a tombstone.  The storage requirements will likely be
> > completely unnoticeable, especially since the map is temporary.
> >
> > Jordan Rose wrote:
> >> On Aug 13, 2012, at 10:22 PM, Ted Kremenek<kremenek at apple.com>  wrote:
> >>
> >>> Hi Richard,
> >>>
> >>> Thanks for making great progress on this!  I agree with Richard
> Smith's suggestion about not using an std::map, but is there a reason to
> not just use a DenseMap to see if there are any enum collisions?
>  Fundamentally your algorithm is O(n*log n), not O(n).  Since you are
> converted everything to an int64_t, could you not just use a
> DenseMap<int64_t, EnumConstantDecl*>?  You could then iterate over the
> EnumConstantDecls in one linear pass, converting them to int64_t's on the
> fly for comparison purposes.  A few advantages I see are:
> >>
> >> The reason Dmitri didn't use a DenseMap for his type-tag work is
> because DenseMap needs to reserve an "empty" value and a "tombstone" value.
> I assume the same problem applies here: all possible int64_t values are
> possible enum constants, and we don't know until we've seen all the enums
> which ones are available as empty or tombstone values. It's possible
> there's a way around this, though.
> > _______________________________________________
> > cfe-commits mailing list
> > cfe-commits at cs.uiuc.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120814/0de5fbf6/attachment.html>