Sorry about the incredible "lateness" of this review, but the error has been bugging me for a while.
On Mon, Jan 7, 2013 at 7:58 PM, Argyrios Kyrtzidis <[email protected]> wrote: > Author: akirtzidis > Date: Mon Jan 7 18:58:25 2013 > New Revision: 171828 > > URL: http://llvm.org/viewvc/llvm-project?rev=171828&view=rev > Log: > [arcmt] Follow-up for r171484; make sure when adding brackets enclosing case > statements, > that the case does not "contain" a declaration that is referenced "outside" > of it, > otherwise we will emit un-compilable code. > > Modified: > cfe/trunk/lib/ARCMigrate/TransProtectedScope.cpp > cfe/trunk/test/ARCMT/checking.m > cfe/trunk/test/ARCMT/protected-scope.m > cfe/trunk/test/ARCMT/protected-scope.m.result > > Modified: cfe/trunk/lib/ARCMigrate/TransProtectedScope.cpp > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/ARCMigrate/TransProtectedScope.cpp?rev=171828&r1=171827&r2=171828&view=diff > ============================================================================== > --- cfe/trunk/lib/ARCMigrate/TransProtectedScope.cpp (original) > +++ cfe/trunk/lib/ARCMigrate/TransProtectedScope.cpp Mon Jan 7 18:58:25 2013 > @@ -15,6 +15,7 @@ > #include "Transforms.h" > #include "Internals.h" > #include "clang/Sema/SemaDiagnostic.h" > +#include "clang/AST/ASTContext.h" > > using namespace clang; > using namespace arcmt; > @@ -22,26 +23,58 @@ > > namespace { > > +class LocalRefsCollector : public RecursiveASTVisitor<LocalRefsCollector> { > + SmallVectorImpl<DeclRefExpr *> &Refs; > + > +public: > + LocalRefsCollector(SmallVectorImpl<DeclRefExpr *> &refs) > + : Refs(refs) { } > + > + bool VisitDeclRefExpr(DeclRefExpr *E) { > + if (ValueDecl *D = E->getDecl()) > + if (D->getDeclContext()->getRedeclContext()->isFunctionOrMethod()) > + Refs.push_back(E); > + return true; > + } > +}; > + > struct CaseInfo { > SwitchCase *SC; > SourceRange Range; > - bool FixedBypass; > + enum { > + St_Unchecked, > + St_CannotFix, > + St_Fixed > + } State; > > - CaseInfo() : SC(0), FixedBypass(false) {} > + CaseInfo() : SC(0), State(St_Unchecked) {} > CaseInfo(SwitchCase *S, SourceRange Range) > - : SC(S), Range(Range), FixedBypass(false) {} > + : SC(S), Range(Range), State(St_Unchecked) {} > }; > > class CaseCollector : public RecursiveASTVisitor<CaseCollector> { > + ParentMap &PMap; > llvm::SmallVectorImpl<CaseInfo> &Cases; > > public: > - CaseCollector(llvm::SmallVectorImpl<CaseInfo> &Cases) > - : Cases(Cases) { } > + CaseCollector(ParentMap &PMap, llvm::SmallVectorImpl<CaseInfo> &Cases) > + : PMap(PMap), Cases(Cases) { } > > bool VisitSwitchStmt(SwitchStmt *S) { > - SourceLocation NextLoc = S->getLocEnd(); > SwitchCase *Curr = S->getSwitchCaseList(); > + if (!Curr) > + return true; > + Stmt *Parent = getCaseParent(Curr); > + Curr = Curr->getNextSwitchCase(); > + // Make sure all case statements are in the same scope. > + while (Curr) { > + if (getCaseParent(Curr) != Parent) > + return true; > + Curr = Curr->getNextSwitchCase(); > + } > + > + SourceLocation NextLoc = S->getLocEnd(); > + Curr = S->getSwitchCaseList(); > // We iterate over case statements in reverse source-order. > while (Curr) { > Cases.push_back(CaseInfo(Curr,SourceRange(Curr->getLocStart(), > NextLoc))); > @@ -50,69 +83,114 @@ > } > return true; > } > -}; > > -} // anonymous namespace > + Stmt *getCaseParent(SwitchCase *S) { > + Stmt *Parent = PMap.getParent(S); > + while (Parent && (isa<SwitchCase>(Parent) || isa<LabelStmt>(Parent))) > + Parent = PMap.getParent(Parent); > + return Parent; > + } > +}; > > -static bool isInRange(FullSourceLoc Loc, SourceRange R) { > - return !Loc.isBeforeInTranslationUnitThan(R.getBegin()) && > - Loc.isBeforeInTranslationUnitThan(R.getEnd()); > -} > +class ProtectedScopeFixer { > + MigrationPass &Pass; > + SourceManager &SM; > + SmallVector<CaseInfo, 16> Cases; > + SmallVector<DeclRefExpr *, 16> LocalRefs; > > -static bool handleProtectedNote(const StoredDiagnostic &Diag, > - llvm::SmallVectorImpl<CaseInfo> &Cases, > - TransformActions &TA) { > - assert(Diag.getLevel() == DiagnosticsEngine::Note); > - > - for (unsigned i = 0; i != Cases.size(); i++) { > - CaseInfo &info = Cases[i]; > - if (isInRange(Diag.getLocation(), info.Range)) { > - TA.clearDiagnostic(Diag.getID(), Diag.getLocation()); > - if (!info.FixedBypass) { > - TA.insertAfterToken(info.SC->getColonLoc(), " {"); > - TA.insert(info.Range.getEnd(), "}\n"); > - info.FixedBypass = true; > +public: > + ProtectedScopeFixer(BodyContext &BodyCtx) > + : Pass(BodyCtx.getMigrationContext().Pass), > + SM(Pass.Ctx.getSourceManager()) { > + > + CaseCollector(BodyCtx.getParentMap(), Cases) > + .TraverseStmt(BodyCtx.getTopStmt()); > + LocalRefsCollector(LocalRefs).TraverseStmt(BodyCtx.getTopStmt()); > + > + SourceRange BodyRange = BodyCtx.getTopStmt()->getSourceRange(); > + const CapturedDiagList &DiagList = Pass.getDiags(); > + CapturedDiagList::iterator I = DiagList.begin(), E = DiagList.end(); > + while (I != E) { > + if (I->getID() == diag::err_switch_into_protected_scope && > + isInRange(I->getLocation(), BodyRange)) { > + handleProtectedScopeError(I, E); > + continue; The use of iterators here is problematic because handleProtectedScopeError can invalidate the iterators. This is causing a failed assertion in debug build of MSVC11 where there are checked iterators involved. >From what I am seeing, handleProtectedScopeError has a Transaction object, which on destruction winds up calling CapturedDiagList::clearDiagnostic, and that calls erase on the list, which invalidates the iterators. Attempting to perform the comparison then fires the assert. I'm not entirely certain of the best way to solve this issue, but I figured I would mention that it causes problems. We have a test case already demonstrating the issue with test\ARCMT\protected-scope.m ~Aaron _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
