[cfe-commits] [PATCH] Fix diagnostic reporting of previous default statement.

[sashan]

>  
> 
> Comparing just based on the line number won't work, since the two cases could
> be in different files, or could appear on the same line.
> SourceManager::isBeforeInTranslationUnit() is the appropriate way to compare
> two source locations.
> 
> That said, I don't think this is the right solution to the problem. I think
> that, instead, we should be processing (and probably also storing) the list of
> case/default statements in source order rather than reverse source order, as
> we currently are. That would address both the problem you're seeing and the
> other problems I had previously alluded to, where we get diagnostics about
> case values in the opposite order we expect.
> 
> 	- Doug

Hi Doug

Thanks for the feedback. I've attached another patch to address the issue. The
patch inserts the switch cases in source order using
SourceManager::isBeforeInTranslation unit to determine the order they should be
inserted in the list. As a result it fixes the diagnostic error/warning messages
in the case of multiple labels in a switch statement, and the overflow warning
you alluded to previously. It passes the clang tests.

Other than that there are few questions about the attached patch that I'm not
sure about as well as the driving philosophy behind some of the code in clang
that maybe you can answer.

1. I've used dyn_cast to determine if the switch case is a DefaultStmt or a
SwitchStmt. The reason for this is so I can call the DefaultStmt::getDefaultLoc
or CaseStmt::getCaseLoc. I believe that it would be better to make a virtual
function in the base class, SwitchCase::getSCLoc, to handle this.

2. Why was a home-brew list used to store the list of switch cases? I would
rather it used std::list. In fact I have another rough non-working patch that
replaces the list with std::set and when new switch cases are added they are
inserted in source order because the set is declared with a functor that decides
the source order of 2 switch cases.

Thanks,

Sashan

-------------- next part --------------
>From 5b93320648361236ed6253e92226ff585ef8502d Mon Sep 17 00:00:00 2001
From: sashan <sashan at zenskg.net>
Date: Thu, 23 Dec 2010 15:57:29 +1100
Subject: [PATCH] Fix diagnostic error/warning messages

This fixes diagnostic error messages where the message would refer to the wrong
line. For example,

  default:;
  default:;

clang would warn about the mulitple default statement but identify the 2nd
default as the previous default statement. It also fixes the overflow warning generated
in the following code:

void f(char a)
{
  switch (a)
  {
    case 1001:break;
    case 1002:break;
  }
}

The fix itself inserts the switch cases (default or case statements) in source
order in the internal list.

diff --git a/include/clang/AST/Stmt.h b/include/clang/AST/Stmt.h
index b3523a1..94c874e 100644
--- a/include/clang/AST/Stmt.h
+++ b/include/clang/AST/Stmt.h
@@ -786,11 +786,7 @@ public:
     SubExprs[BODY] = S;
     SwitchLoc = SL;
   }
-  void addSwitchCase(SwitchCase *SC) {
-    assert(!SC->getNextSwitchCase() && "case/default already added to a switch");
-    SC->setNextSwitchCase(FirstCase);
-    FirstCase = SC;
-  }
+  void addSwitchCase(SwitchCase *SC, SourceLocation &Loc, SourceManager& SM);
 
   /// Set a flag in the SwitchStmt indicating that if the 'switch (X)' is a
   /// switch over an enum value then all cases have been explicitly covered.
diff --git a/lib/AST/Stmt.cpp b/lib/AST/Stmt.cpp
index acd77be..246da5a 100644
--- a/lib/AST/Stmt.cpp
+++ b/lib/AST/Stmt.cpp
@@ -20,7 +20,9 @@
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/ASTDiagnostic.h"
 #include "clang/Basic/TargetInfo.h"
+#include "clang/Basic/SourceManager.h"
 #include <cstdio>
+#include "llvm/Support/raw_ostream.h"
 using namespace clang;
 
 static struct StmtClassNameTable {
@@ -537,6 +539,58 @@ SwitchStmt::SwitchStmt(ASTContext &C, VarDecl *Var, Expr *cond)
   SubExprs[BODY] = NULL;
 }
 
+void SwitchStmt::addSwitchCase(SwitchCase *SC, SourceLocation &Loc,
+                               SourceManager &SM) {
+  assert(!SC->getNextSwitchCase() && "case/default already added to a switch");
+  if (!FirstCase) {
+    // This is the first switch case added to the list
+    FirstCase = SC;
+    return;
+  }
+
+  SwitchCase *Current = FirstCase;
+  SwitchCase *Previous = NULL;
+  // Insert the switch case in source order.
+  while (Current) {
+    // Work out if the current switch case is a default stmt or a case stmt
+    SourceLocation LocCurrent;
+    const CaseStmt *CS = dyn_cast<CaseStmt>(Current);
+    if (CS) {
+      LocCurrent = CS->getCaseLoc();
+    }
+    const DefaultStmt *DS = dyn_cast<DefaultStmt>(Current);
+    if (DS) {
+      LocCurrent = DS->getDefaultLoc();
+    }
+
+    // A switch case would have to be either a case stmt or a default stmt. If
+    // it's neither then there is something else wrong...
+    assert(DS || CS);
+
+    if (SM.isBeforeInTranslationUnit(LocCurrent, Loc)) {
+      // Go to the next switch case in the list.
+      Previous = Current;
+      Current = Current->getNextSwitchCase();
+    } else {
+      // Insert the new switch case.
+      SC->setNextSwitchCase(Current);
+      if (Previous) {
+        Previous->setNextSwitchCase(SC);
+      } else {
+        // No previous switch case therefore this must be the new switch case at
+        // the start of the list.
+        FirstCase = SC;
+      }
+      return;
+    }
+  }
+  if (!Current) {
+    // At the end of the list so the new switch case must come after all the
+    // others.
+    Previous->setNextSwitchCase(SC);
+  }
+}
+
 VarDecl *SwitchStmt::getConditionVariable() const {
   if (!SubExprs[VAR])
     return 0;
diff --git a/lib/Sema/SemaStmt.cpp b/lib/Sema/SemaStmt.cpp
index 6ddbe13..6f7810e 100644
--- a/lib/Sema/SemaStmt.cpp
+++ b/lib/Sema/SemaStmt.cpp
@@ -208,7 +208,7 @@ Sema::ActOnCaseStmt(SourceLocation CaseLoc, Expr *LHSVal,
 
   CaseStmt *CS = new (Context) CaseStmt(LHSVal, RHSVal, CaseLoc, DotDotDotLoc,
                                         ColonLoc);
-  getCurFunction()->SwitchStack.back()->addSwitchCase(CS);
+  getCurFunction()->SwitchStack.back()->addSwitchCase(CS, CaseLoc, SourceMgr);
   return Owned(CS);
 }
 
@@ -227,7 +227,7 @@ Sema::ActOnDefaultStmt(SourceLocation DefaultLoc, SourceLocation ColonLoc,
   }
 
   DefaultStmt *DS = new (Context) DefaultStmt(DefaultLoc, ColonLoc, SubStmt);
-  getCurFunction()->SwitchStack.back()->addSwitchCase(DS);
+  getCurFunction()->SwitchStack.back()->addSwitchCase(DS, DefaultLoc, SourceMgr);
   return Owned(DS);
 }
 
diff --git a/test/CXX/stmt.stmt/stmt.label/p1.cpp b/test/CXX/stmt.stmt/stmt.label/p1.cpp
index f2ace35..9efef9b 100644
--- a/test/CXX/stmt.stmt/stmt.label/p1.cpp
+++ b/test/CXX/stmt.stmt/stmt.label/p1.cpp
@@ -19,7 +19,7 @@ void h()
   switch (x)
   {
     case 1:;
-    default:; // expected-error{{multiple default labels in one switch}}
     default:; // expected-note{{previous case defined here}}
+    default:; // expected-error{{multiple default labels in one switch}}
   }
 }
-- 
1.7.2.3