<html><head><meta http-equiv="Content-Type" content="text/html charset=us-ascii"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;">On Mar 12, 2013, at 10:48 AM, Aaron Ballman <<a href="mailto:aaron@aaronballman.com">aaron@aaronballman.com</a>> wrote:<br><div><br class="Apple-interchange-newline"><blockquote type="cite"><div style="letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px;">Have you had the chance to look into this? It's still unresolved for<br>me, and causes the MSVC 11 debug builds to fail.<br></div></blockquote><div><br></div><div>No sorry. This week is pretty busy not sure I can look into it this week.</div><div><br></div><div>-Argyrios</div><br><blockquote type="cite"><div style="letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px;"><br>Thanks!<br><br>~Aaron<br><br>On Mon, Feb 18, 2013 at 10:26 PM, Argyrios Kyrtzidis <<a href="mailto:akyrtzi@gmail.com">akyrtzi@gmail.com</a>> wrote:<br><blockquote type="cite">Thanks for letting me know Aaron, I'll look into it.<br><br>-Argyrios<br><br>On Feb 18, 2013, at 5:59 PM, Aaron Ballman <<a href="mailto:aaron@aaronballman.com">aaron@aaronballman.com</a>> wrote:<br><br><blockquote type="cite">Sorry about the incredible "lateness" of this review, but the error<br>has been bugging me for a while.<br><br>On Mon, Jan 7, 2013 at 7:58 PM, Argyrios Kyrtzidis <<a href="mailto:akyrtzi@gmail.com">akyrtzi@gmail.com</a>> wrote:<br><blockquote type="cite">Author: akirtzidis<br>Date: Mon Jan 7 18:58:25 2013<br>New Revision: 171828<br><br>URL: <a href="http://llvm.org/viewvc/llvm-project?rev=171828&view=rev">http://llvm.org/viewvc/llvm-project?rev=171828&view=rev</a><br>Log:<br>[arcmt] Follow-up for r171484; make sure when adding brackets enclosing case statements,<br>that the case does not "contain" a declaration that is referenced "outside" of it,<br>otherwise we will emit un-compilable code.<br><br>Modified:<br> cfe/trunk/lib/ARCMigrate/TransProtectedScope.cpp<br> cfe/trunk/test/ARCMT/checking.m<br> cfe/trunk/test/ARCMT/protected-scope.m<br> cfe/trunk/test/ARCMT/protected-scope.m.result<br><br>Modified: cfe/trunk/lib/ARCMigrate/TransProtectedScope.cpp<br>URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/ARCMigrate/TransProtectedScope.cpp?rev=171828&r1=171827&r2=171828&view=diff">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/ARCMigrate/TransProtectedScope.cpp?rev=171828&r1=171827&r2=171828&view=diff</a><br>==============================================================================<br>--- cfe/trunk/lib/ARCMigrate/TransProtectedScope.cpp (original)<br>+++ cfe/trunk/lib/ARCMigrate/TransProtectedScope.cpp Mon Jan 7 18:58:25 2013<br>@@ -15,6 +15,7 @@<br>#include "Transforms.h"<br>#include "Internals.h"<br>#include "clang/Sema/SemaDiagnostic.h"<br>+#include "clang/AST/ASTContext.h"<br><br>using namespace clang;<br>using namespace arcmt;<br>@@ -22,26 +23,58 @@<br><br>namespace {<br><br>+class LocalRefsCollector : public RecursiveASTVisitor<LocalRefsCollector> {<br>+ SmallVectorImpl<DeclRefExpr *> &Refs;<br>+<br>+public:<br>+ LocalRefsCollector(SmallVectorImpl<DeclRefExpr *> &refs)<br>+ : Refs(refs) { }<br>+<br>+ bool VisitDeclRefExpr(DeclRefExpr *E) {<br>+ if (ValueDecl *D = E->getDecl())<br>+ if (D->getDeclContext()->getRedeclContext()->isFunctionOrMethod())<br>+ Refs.push_back(E);<br>+ return true;<br>+ }<br>+};<br>+<br>struct CaseInfo {<br> SwitchCase *SC;<br> SourceRange Range;<br>- bool FixedBypass;<br>+ enum {<br>+ St_Unchecked,<br>+ St_CannotFix,<br>+ St_Fixed<br>+ } State;<br><br>- CaseInfo() : SC(0), FixedBypass(false) {}<br>+ CaseInfo() : SC(0), State(St_Unchecked) {}<br> CaseInfo(SwitchCase *S, SourceRange Range)<br>- : SC(S), Range(Range), FixedBypass(false) {}<br>+ : SC(S), Range(Range), State(St_Unchecked) {}<br>};<br><br>class CaseCollector : public RecursiveASTVisitor<CaseCollector> {<br>+ ParentMap &PMap;<br> llvm::SmallVectorImpl<CaseInfo> &Cases;<br><br>public:<br>- CaseCollector(llvm::SmallVectorImpl<CaseInfo> &Cases)<br>- : Cases(Cases) { }<br>+ CaseCollector(ParentMap &PMap, llvm::SmallVectorImpl<CaseInfo> &Cases)<br>+ : PMap(PMap), Cases(Cases) { }<br><br> bool VisitSwitchStmt(SwitchStmt *S) {<br>- SourceLocation NextLoc = S->getLocEnd();<br> SwitchCase *Curr = S->getSwitchCaseList();<br>+ if (!Curr)<br>+ return true;<br>+ Stmt *Parent = getCaseParent(Curr);<br>+ Curr = Curr->getNextSwitchCase();<br>+ // Make sure all case statements are in the same scope.<br>+ while (Curr) {<br>+ if (getCaseParent(Curr) != Parent)<br>+ return true;<br>+ Curr = Curr->getNextSwitchCase();<br>+ }<br>+<br>+ SourceLocation NextLoc = S->getLocEnd();<br>+ Curr = S->getSwitchCaseList();<br> // We iterate over case statements in reverse source-order.<br> while (Curr) {<br> Cases.push_back(CaseInfo(Curr,SourceRange(Curr->getLocStart(), NextLoc)));<br>@@ -50,69 +83,114 @@<br> }<br> return true;<br> }<br>-};<br><br>-} // anonymous namespace<br>+ Stmt *getCaseParent(SwitchCase *S) {<br>+ Stmt *Parent = PMap.getParent(S);<br>+ while (Parent && (isa<SwitchCase>(Parent) || isa<LabelStmt>(Parent)))<br>+ Parent = PMap.getParent(Parent);<br>+ return Parent;<br>+ }<br>+};<br><br>-static bool isInRange(FullSourceLoc Loc, SourceRange R) {<br>- return !Loc.isBeforeInTranslationUnitThan(R.getBegin()) &&<br>- Loc.isBeforeInTranslationUnitThan(R.getEnd());<br>-}<br>+class ProtectedScopeFixer {<br>+ MigrationPass &Pass;<br>+ SourceManager &SM;<br>+ SmallVector<CaseInfo, 16> Cases;<br>+ SmallVector<DeclRefExpr *, 16> LocalRefs;<br><br>-static bool handleProtectedNote(const StoredDiagnostic &Diag,<br>- llvm::SmallVectorImpl<CaseInfo> &Cases,<br>- TransformActions &TA) {<br>- assert(Diag.getLevel() == DiagnosticsEngine::Note);<br>-<br>- for (unsigned i = 0; i != Cases.size(); i++) {<br>- CaseInfo &info = Cases[i];<br>- if (isInRange(Diag.getLocation(), info.Range)) {<br>- TA.clearDiagnostic(Diag.getID(), Diag.getLocation());<br>- if (!info.FixedBypass) {<br>- TA.insertAfterToken(info.SC->getColonLoc(), " {");<br>- TA.insert(info.Range.getEnd(), "}\n");<br>- info.FixedBypass = true;<br>+public:<br>+ ProtectedScopeFixer(BodyContext &BodyCtx)<br>+ : Pass(BodyCtx.getMigrationContext().Pass),<br>+ SM(Pass.Ctx.getSourceManager()) {<br>+<br>+ CaseCollector(BodyCtx.getParentMap(), Cases)<br>+ .TraverseStmt(BodyCtx.getTopStmt());<br>+ LocalRefsCollector(LocalRefs).TraverseStmt(BodyCtx.getTopStmt());<br>+<br>+ SourceRange BodyRange = BodyCtx.getTopStmt()->getSourceRange();<br>+ const CapturedDiagList &DiagList = Pass.getDiags();<br>+ CapturedDiagList::iterator I = DiagList.begin(), E = DiagList.end();<br>+ while (I != E) {<br>+ if (I->getID() == diag::err_switch_into_protected_scope &&<br>+ isInRange(I->getLocation(), BodyRange)) {<br>+ handleProtectedScopeError(I, E);<br>+ continue;<br></blockquote><br>The use of iterators here is problematic because<br>handleProtectedScopeError can invalidate the iterators. This is<br>causing a failed assertion in debug build of MSVC11 where there are<br>checked iterators involved.<br><br>From what I am seeing, handleProtectedScopeError has a Transaction<br>object, which on destruction winds up calling<br>CapturedDiagList::clearDiagnostic, and that calls erase on the list,<br>which invalidates the iterators. Attempting to perform the comparison<br>then fires the assert.<br><br>I'm not entirely certain of the best way to solve this issue, but I<br>figured I would mention that it causes problems. We have a test case<br>already demonstrating the issue with test\ARCMT\protected-scope.m<br><br>~Aaron</blockquote></blockquote></div></blockquote></div><br></body></html>