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

[sashan]

On Thu Dec 23 07:40:49 2010, Douglas Gregor wrote:
> 
> On Dec 22, 2010, at 9:25 PM, sashan wrote:
> 
> >> 
> >> 
> >> 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.
> 
> 
> The dyn_cast is fine here. We don't want a virtual because we're slowly
> eliminating virtuals from the AST.
> 
> Your newer patch is technically correct. However, it has severe performance
> implications for switch statements with many cases, because
> SwitchStmt::addSwitchCase() has gone from O(1) to O(N).
> 
> I had said before:
> 
> >> 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.
> 
> 
> It's still not the right solution to look at the source locations. Just
> reverse the list and we'll have the cases in source order.
> 

Reversing the list was what I originally (see
http://lists.cs.uiuc.edu/pipermail/cfe-dev/2010-December/012387.html) did but it
doesn't work for this sort of statement:

  default:
  default:;

Anyway I've attached a patch that effectively reverses the list.

-------------- next part --------------
>From 2c13bc5ae5166831f0318a79828464259b6ca030 Mon Sep 17 00:00:00 2001
From: sashan <sashan at zenskg.net>
Date: Thu, 30 Dec 2010 19:43:36 +1100
Subject: [PATCH] Store the case/default stmts at the end of the list.

Previous code would prepend the case/default statements to the front of the
list. This change reverses the ordering by placing them at the end of the list.

diff --git a/include/clang/AST/Stmt.h b/include/clang/AST/Stmt.h
index b3523a1..538783e 100644
--- a/include/clang/AST/Stmt.h
+++ b/include/clang/AST/Stmt.h
@@ -738,6 +738,7 @@ class SwitchStmt : public Stmt {
   Stmt* SubExprs[END_EXPR];
   // This points to a linked list of case and default statements.
   SwitchCase *FirstCase;
+  SwitchCase *LastCase;
   SourceLocation SwitchLoc;
 
   /// If the SwitchStmt is a switch on an enum value, this records whether
@@ -786,11 +787,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);
 
   /// 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..2b0087a 100644
--- a/lib/AST/Stmt.cpp
+++ b/lib/AST/Stmt.cpp
@@ -77,6 +77,7 @@ void Stmt::addStmtClass(StmtClass s) {
   ++getStmtInfoTableEntry(s).Counter;
 }
 
+
 static bool StatSwitch = false;
 
 bool Stmt::CollectingStats(bool Enable) {
@@ -530,13 +531,25 @@ void ForStmt::setConditionVariable(ASTContext &C, VarDecl *V) {
 }
 
 SwitchStmt::SwitchStmt(ASTContext &C, VarDecl *Var, Expr *cond) 
-  : Stmt(SwitchStmtClass), FirstCase(0), AllEnumCasesCovered(0) 
+  : Stmt(SwitchStmtClass), FirstCase(0), LastCase(0),
+    AllEnumCasesCovered(0)
 {
   setConditionVariable(C, Var);
   SubExprs[COND] = reinterpret_cast<Stmt*>(cond);
   SubExprs[BODY] = NULL;
 }
 
+void SwitchStmt::addSwitchCase(SwitchCase *SC) {
+  assert(!SC->getNextSwitchCase() && "case/default already added to a switch");
+  if (!FirstCase) {
+    FirstCase = SC;
+    LastCase = SC;
+  } else {
+    LastCase->setNextSwitchCase(SC);
+    LastCase = SC;
+  }
+}
+
 VarDecl *SwitchStmt::getConditionVariable() const {
   if (!SubExprs[VAR])
     return 0;
-- 
1.7.3.4