-Wswitch warning patch

Hi,
the attached patches implement gcc's -Wswitch and add simple tests for it.

Michal.

DiagnosticSemaKinds.td.patch (724 Bytes)

switch.c.patch (748 Bytes)

SemaStmt.cpp.patch (1.34 KB)

There's something strange with your patch. 'EnumVals' is filled in, but never used.
Also, please add a space between 'if'/'for' and '('.

Nuno

I *love* this warning. Thanks for working on it!

The logic in Sema::ActOnFinishSwitchStmt needs a bit of work, though:

+ if(!TheDefaultStmt && ET) {
+ const EnumDecl *ED = ET->getDecl();
+ llvm::SmallVector<EnumConstantDecl*, 64> EnumVals;

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.

- 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.

Michal.

Wswitch.patch (4.9 KB)

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

Attaching (I hope) final version.

Wswitch.patch (5.18 KB)

Makes sense. The patch looks good, and I can commit it; do you have test cases?

  - Doug

Makes sense. The patch looks good, and I can commit it; do you have test cases?

Few tests are in attachment.

switch.c.patch (2.75 KB)

Thanks! I've committed your patch as r95592. The only thing I changed was to add the warning group -Wswitch-enum (which is also enabled by -Wswitch), and to associate your new warnings with -Wswitch-enum. That way, these warnings can be selectively turned on/off.

  - Doug