Have you had the chance to look into this? It's still unresolved for me, and causes the MSVC 11 debug builds to fail.
Thanks! ~Aaron On Mon, Feb 18, 2013 at 10:26 PM, Argyrios Kyrtzidis <[email protected]> wrote: > Thanks for letting me know Aaron, I'll look into it. > > -Argyrios > > On Feb 18, 2013, at 5:59 PM, Aaron Ballman <[email protected]> wrote: > >> 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
