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.
>From 2c13bc5ae5166831f0318a79828464259b6ca030 Mon Sep 17 00:00:00 2001 From: sashan <[email protected]> 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
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
