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

[Richard Smith]

+  // Use to store the smallest APSInt size that can represent all the
elements.
+  bool signedness = false;
+  unsigned bitwidth = 0;
+
+  // Skip diagnostic if previous error were found with the enum.
+  for (unsigned i = 0; i != NumElements; ++i) {
+    EnumConstantDecl *ECD = cast<EnumConstantDecl>(Elements[i]);
+    if (!ECD)
+      return;
+
+    const llvm::APSInt& Val = ECD->getInitVal();
+    if (!signedness && Val.isSigned()) {
+      signedness = true;
+     ++bitwidth;
+    }
+
+    unsigned ValWidth;
+    if (Val.isUnsigned())
+      ValWidth = Val.getActiveBits() + signedness;
+    else if (Val.isNonNegative())
+      ValWidth = Val.getActiveBits() + 1;
+    else
+      ValWidth = Val.getBitWidth() - Val.countLeadingOnes() + 1;
+
+    if (bitwidth < ValWidth)
+      bitwidth = ValWidth;
+  }

You can get these directly from the EnumDecl: see getNumPositiveBits,
getNumNegativeBits.

+  // Store a map of values to decls.  Values are extended to a common size
+  // first to for comparisons.
+  std::map<llvm::APSInt, EnumConstantDecl*> ValueMap;

How about building a SmallVector of EnumConstantDecl*, sorting it by init
value then by whether there is an init expression, and finally performing
your check as a single linear pass over the vector? That should be a bit
quicker than the repeated map lookups and heap allocations you're currently
doing.

On Wed, Jul 25, 2012 at 2:37 PM, Ted Kremenek <kremenek at apple.com> wrote:

> Ok.  That's still a scary number.  Do you have numbers for realistic
> examples?  For example, we know Clang has some particularly large enums.
>  This micro benchmark is useful, but it may be overly pessimistic.
>
> On Jul 25, 2012, at 2:34 PM, Richard Trieu <rtrieu at google.com> wrote:
>
> Yes, that is the slowdown for the entire -fsyntax-only time for a source
> file with only an enum in it.
>
> On Wed, Jul 25, 2012 at 1:48 PM, Ted Kremenek <kremenek at apple.com> wrote:
>
>> Hi Richard,
>>
>> If I am reading that right, the 6-10% slowdown is for the entire
>> -fsyntax-only time?  If so, that's definitely cost prohibitive.
>>
>> Ted
>>
>> On Jul 19, 2012, at 8:25 PM, Richard Trieu <rtrieu at google.com> wrote:
>>
>> On Wed, Jul 18, 2012 at 9:14 PM, Ted Kremenek <kremenek at apple.com> wrote:
>>
>>> On Jul 18, 2012, at 6:34 PM, Richard Trieu <rtrieu at google.com> wrote:
>>>
>>> A set could work for detecting the values, but both EnumConstantDecls
>>> are needed for the diagnostic, not just the values.  Possibly a map from
>>> APSInt->EnumConstantDecl* would work.  But either way, I would be dealing
>>> with getting APSInts to play nice with each other.
>>>
>>>
>>> That seems reasonable to me.  The primary performance issue I see is the
>>> quadratic algorithmic complexity.  If the APSInt comparisons are an issue,
>>> we can see if we can find ways to optimize that further.
>>>
>>
>> I created two more variations on and measured some timings.  Both used a
>> map, one with a custom compare function and one that extended the APSInt
>> value before insertion.  The APSInt extension had the better time, so I'll
>> be giving the number for that one.
>>
>> At 10,000 elements, there was a 6-10% slow down.  This amounts to .01-.03
>> seconds difference on .13-.27 second runtime.
>>
>> At 100,000 elements, 8-12% slow down.  .2-.3 seconds on 1.34 to 2.66
>> second run time.
>>
>> At 1,000,000 elements, 7-14% slow down.  Around 2 second difference for
>> runs of 13.6 to 26.7 seconds.
>>
>> A new patch has been attached which has the APSInt bit extension before
>> adding to the map.
>> <duplicate-enum-bit-extension.patch>
>>
>>
>>
>
>
> _______________________________________________
> 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/20120725/51f533f8/attachment.html>