[cfe-dev] -Wswitch warning patch

[Douglas Gregor]

On Jan 16, 2010, at 6:49 AM, Michal wrote:

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

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;
+
+      // Gather all enum values and sort them, allowing easier  
comparison
+      // with CaseVals.
+      EnumVals.append(ED->enumerator_begin(), ED->enumerator_end());
+      std::sort(EnumVals.begin(), EnumVals.end());

This sort is based on the address of the EnumConstantDecl; that  
ordering isn't stable from one execution of Clang to the next, so we  
should not depend on it. You'll want to sort by the value of each  
enumerator. And, as Nuno noted, EnumVals doesn't seem to be used...  
although I suspect you meant to use it in the loop below.

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

+          Diag(CondExpr->getExprLoc(), diag::warn_missing_cases) <<  
EI->getNameAsCString();
+        else if(EI->getInitVal().sle(CI->first))
+          CI++;
+      }

A few things:

   - This isn't accounting for CaseRanges (the GNU case-range  
extension), which can cover multiple enumerators in a single case

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

	- Doug