[cfe-dev] -Wswitch warning patch

[Douglas Gregor]

On Jan 23, 2010, at 11:59 AM, Michal wrote:

> Thank you both for replies. Attaching a newer, but not yet finished version.
> 
> 
>> 
>> +      // Iterate over Enum values and check which are missing in switch
>> +      CaseValsTy::const_iterator CI = CaseVals.begin();
>> +      for(EnumDecl::enumerator_iterator EI = ED->enumerator_begin(); EI !=
>> ED->enumerator_end(); EI++) {
>> +        if(CI == CaseVals.end() || EI->getInitVal().slt(CI->first))
>> 
>> Why use "slt" here? The values aren't guaranteed to be signed.
> I misunderstood slt code. I already asked here how to compare ASPInts but
> I am not sure whether it is appropriate to use your answer here. Are you ok with
> adding another function that copies and extends an ASPInt, and calling it for
> each comparison?
> As for now I am using ordinary <,==, etc, which obviously breaks if enum has no
> negative values.

Every enumeration has an underlying integral type. You can ask that type whether it is signed or unsigned, and use ASTContext's getTypeSize() to determine its size (in bits). From that information, construct the APSInts and compare them with ==.

>>  - There may be multiple enumeration constants with the same value
>> (FirstDecl = 1, VarDecl = 1, FunctionDecl = 2, etc.); please make sure that
>> we do not warn more than once if both FirstDecl and VarDecl are missing, and
>> that the presence of such enumerators does not cause spurious warnings.
>> (std::unique could eliminate duplicates in the list of enumerators).
>> 
>>  - Try to make sure that recovery is good; if we complain about the second
>> enumerator missing, it shouldn't cause a cascade of failures.
>> 
> Not sure if I understood you correctly, if a switch misses more than two
> different values should we report only the first two? If so, I think
> that at least
> there should be a warning notifying that there are more, but just not
> printed, missing
> enum constants.

Sorry, I wasn't clear. All I meant is that, when I looked at the previous patch, it looked like it would get confused after seeing the first missing enum value and then print warnings about missing enum values that were actually there. I may have misunderstood the code.

+      EnumVals.append(ED->enumerator_begin(), ED->enumerator_end());
+      std::sort(EnumVals.begin(), EnumVals.end(), CmpEnumVals);

Good, this is sorting by enumerator values. Please use std::stable_sort so that we get a deterministic sort across platforms, ensuring that we always complain about the first missing enumerator with a given value. For example, if we have:

  enum Blah {
    DeclRefExpr = 20,
    // ...
    FirstExpr = 20
  };

we'll complain about DeclRefExpr when it's missing, not FirstExpr.

+            Diag(CI->second->getLHS()->getExprLoc(), diag::not_in_enum) << ED->getNameAsCString();

Instead of ED->getNameAsCString(), please use ED->getDeclName(). It doesn't matter for enums (which can't have funky names), but it's good style.

This patch is looking good. I look forward to reviewing the final version.

	- Doug
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20100128/e7f46210/attachment.html>