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

Ted Kremenek kremenek at apple.com
Tue Aug 14 21:45:27 PDT 2012


On Aug 14, 2012, at 2:32 PM, Richard Trieu <rtrieu at google.com> wrote:

> I think it's best to back up a little in the loop.
> 
>  +    llvm::APSInt Value = (*I)->getInitVal();
> 
> At this point, a valid enum is found for the warning, which is stored in I.  Now, from here to the end of this iteration of the for-loop, process every enum which has the same Value.
> 
> With the current approach, I'm not certain, but something about the following looks curious:
> 
>> +    // Find the first duplicate enum.
>> +    llvm::SmallVector<EnumConstantDecl*, 10>::iterator Iter = I + 1;
>> +    for (; Iter != E && llvm::APSInt::isSameValue(Value, (*Iter)->getInitVal());
>> +         ++Iter) {
>> +      if (ValidDuplicateEnum(*Iter, Enum))
>> +        break;
>> +    }
>> +
>> 
> 
> This loop is similar to the next loop.  Before emitting the warning, check that there exists a enum to put into a loop.  If an enum constant is found, break, emitting the warning, then start the loop again, emitting notes.
> 
> 
> If the array is sorted by constant value, I don't believe there is any need to keep marching through the rest of the items if the next item isn't a match.  I may be mistaken, but I think this leaves your implementation still being quadratic in the worst case.  
> All the enums of the same value is handled at one go.  At the end, I is set to Iter so that the next iteration will skip all same value enums and starts at the next value.  This keeps the vector transverse at linear time.
>  

Ah, makes sense!  Thanks so much for the explanation.  The vector traverse is O(n), but the sorting remains O(n log n).  I'm wondering if that is showing up as an algorithmic bottleneck in the test cases you created.  That's why I wonder if a strictly linear algorithm (albeit based on a DenseMap) makes more sense.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120814/a929ff18/attachment.html>


More information about the cfe-commits mailing list