On Jan 4, 2013, at 6:06 PM, Jordan Rose <[email protected]> wrote:
> This is awesome. Would it be generally useful as a fixit for the protected > scope problem in C++? > > Jordan Talked a bit with Jordan about this, and I'm not sure this would work well as a fixit. Fixits are mostly restricted to one line (or very close), I don't think we have any diagnostic currently that causes fixits with a lot of lines between them (or between the point where the diagnostic was emitted) in real code. Maybe we will eventually have a way to trigger more "heavy duty refactorings" than what fixits would allow (for example, instead of only "did you mean 'this'", also have "create a method with that name"), and such an error could be fixed through that. Let me know if anyone else has some thoughts on the matter. -Argyrios > > > On Jan 4, 2013, at 10:30 , Argyrios Kyrtzidis <[email protected]> wrote: > >> Author: akirtzidis >> Date: Fri Jan 4 12:30:08 2013 >> New Revision: 171484 >> >> URL: http://llvm.org/viewvc/llvm-project?rev=171484&view=rev >> Log: >> [arcmt] Adds brackets in case statements that "contain" initialization of >> retaining >> variable, thus emitting the "switch case is in protected scope" error. >> >> rdar://12952016 >> >> Added: >> cfe/trunk/lib/ARCMigrate/TransProtectedScope.cpp >> cfe/trunk/test/ARCMT/protected-scope.m >> cfe/trunk/test/ARCMT/protected-scope.m.result >> Modified: >> cfe/trunk/lib/ARCMigrate/ARCMT.cpp >> cfe/trunk/lib/ARCMigrate/Internals.h >> cfe/trunk/lib/ARCMigrate/Transforms.cpp >> cfe/trunk/lib/ARCMigrate/Transforms.h >> cfe/trunk/test/ARCMT/checking.m >> >> Modified: cfe/trunk/lib/ARCMigrate/ARCMT.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/ARCMigrate/ARCMT.cpp?rev=171484&r1=171483&r2=171484&view=diff >> ============================================================================== >> --- cfe/trunk/lib/ARCMigrate/ARCMT.cpp (original) >> +++ cfe/trunk/lib/ARCMigrate/ARCMT.cpp Fri Jan 4 12:30:08 2013 >> @@ -39,8 +39,9 @@ >> diagLoc.isBeforeInTranslationUnitThan(range.getEnd()))) { >> cleared = true; >> ListTy::iterator eraseS = I++; >> - while (I != List.end() && I->getLevel() == DiagnosticsEngine::Note) >> - ++I; >> + if (eraseS->getLevel() != DiagnosticsEngine::Note) >> + while (I != List.end() && I->getLevel() == DiagnosticsEngine::Note) >> + ++I; >> // Clear the diagnostic and any notes following it. >> I = List.erase(eraseS, I); >> continue; >> @@ -296,7 +297,8 @@ >> std::vector<SourceLocation> ARCMTMacroLocs; >> >> TransformActions testAct(*Diags, capturedDiags, Ctx, >> Unit->getPreprocessor()); >> - MigrationPass pass(Ctx, OrigGCMode, Unit->getSema(), testAct, >> ARCMTMacroLocs); >> + MigrationPass pass(Ctx, OrigGCMode, Unit->getSema(), testAct, >> capturedDiags, >> + ARCMTMacroLocs); >> pass.setNSAllocReallocError(NoNSAllocReallocError); >> pass.setNoFinalizeRemoval(NoFinalizeRemoval); >> >> @@ -599,7 +601,7 @@ >> Rewriter rewriter(Ctx.getSourceManager(), Ctx.getLangOpts()); >> TransformActions TA(*Diags, capturedDiags, Ctx, Unit->getPreprocessor()); >> MigrationPass pass(Ctx, OrigCI.getLangOpts()->getGC(), >> - Unit->getSema(), TA, ARCMTMacroLocs); >> + Unit->getSema(), TA, capturedDiags, ARCMTMacroLocs); >> >> trans(pass); >> >> >> Modified: cfe/trunk/lib/ARCMigrate/Internals.h >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/ARCMigrate/Internals.h?rev=171484&r1=171483&r2=171484&view=diff >> ============================================================================== >> --- cfe/trunk/lib/ARCMigrate/Internals.h (original) >> +++ cfe/trunk/lib/ARCMigrate/Internals.h Fri Jan 4 12:30:08 2013 >> @@ -146,16 +146,20 @@ >> MigratorOptions MigOptions; >> Sema &SemaRef; >> TransformActions &TA; >> + const CapturedDiagList &CapturedDiags; >> std::vector<SourceLocation> &ARCMTMacroLocs; >> llvm::Optional<bool> EnableCFBridgeFns; >> >> MigrationPass(ASTContext &Ctx, LangOptions::GCMode OrigGCMode, >> Sema &sema, TransformActions &TA, >> + const CapturedDiagList &capturedDiags, >> std::vector<SourceLocation> &ARCMTMacroLocs) >> : Ctx(Ctx), OrigGCMode(OrigGCMode), MigOptions(), >> - SemaRef(sema), TA(TA), >> + SemaRef(sema), TA(TA), CapturedDiags(capturedDiags), >> ARCMTMacroLocs(ARCMTMacroLocs) { } >> >> + const CapturedDiagList &getDiags() const { return CapturedDiags; } >> + >> bool isGCMigration() const { return OrigGCMode != LangOptions::NonGC; } >> bool noNSAllocReallocError() const { return >> MigOptions.NoNSAllocReallocError; } >> void setNSAllocReallocError(bool val) { MigOptions.NoNSAllocReallocError = >> val; } >> >> Added: cfe/trunk/lib/ARCMigrate/TransProtectedScope.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/ARCMigrate/TransProtectedScope.cpp?rev=171484&view=auto >> ============================================================================== >> --- cfe/trunk/lib/ARCMigrate/TransProtectedScope.cpp (added) >> +++ cfe/trunk/lib/ARCMigrate/TransProtectedScope.cpp Fri Jan 4 12:30:08 2013 >> @@ -0,0 +1,118 @@ >> +//===--- TransProtectedScope.cpp - Transformations to ARC mode >> ------------===// >> +// >> +// The LLVM Compiler Infrastructure >> +// >> +// This file is distributed under the University of Illinois Open Source >> +// License. See LICENSE.TXT for details. >> +// >> +//===----------------------------------------------------------------------===// >> +// >> +// Adds brackets in case statements that "contain" initialization of >> retaining >> +// variable, thus emitting the "switch case is in protected scope" error. >> +// >> +//===----------------------------------------------------------------------===// >> + >> +#include "Transforms.h" >> +#include "Internals.h" >> +#include "clang/Sema/SemaDiagnostic.h" >> + >> +using namespace clang; >> +using namespace arcmt; >> +using namespace trans; >> + >> +namespace { >> + >> +struct CaseInfo { >> + SwitchCase *SC; >> + SourceRange Range; >> + bool FixedBypass; >> + >> + CaseInfo() : SC(0), FixedBypass(false) {} >> + CaseInfo(SwitchCase *S, SourceRange Range) >> + : SC(S), Range(Range), FixedBypass(false) {} >> +}; >> + >> +class CaseCollector : public RecursiveASTVisitor<CaseCollector> { >> + llvm::SmallVectorImpl<CaseInfo> &Cases; >> + >> +public: >> + CaseCollector(llvm::SmallVectorImpl<CaseInfo> &Cases) >> + : Cases(Cases) { } >> + >> + bool VisitSwitchStmt(SwitchStmt *S) { >> + SourceLocation NextLoc = S->getLocEnd(); >> + SwitchCase *Curr = S->getSwitchCaseList(); >> + // We iterate over case statements in reverse source-order. >> + while (Curr) { >> + Cases.push_back(CaseInfo(Curr,SourceRange(Curr->getLocStart(), >> NextLoc))); >> + NextLoc = Curr->getLocStart(); >> + Curr = Curr->getNextSwitchCase(); >> + } >> + return true; >> + } >> +}; >> + >> +} // anonymous namespace >> + >> +static bool isInRange(FullSourceLoc Loc, SourceRange R) { >> + return !Loc.isBeforeInTranslationUnitThan(R.getBegin()) && >> + Loc.isBeforeInTranslationUnitThan(R.getEnd()); >> +} >> + >> +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; >> + } >> + return true; >> + } >> + } >> + >> + return false; >> +} >> + >> +static void handleProtectedScopeError(CapturedDiagList::iterator &DiagI, >> + CapturedDiagList::iterator DiagE, >> + llvm::SmallVectorImpl<CaseInfo> >> &Cases, >> + TransformActions &TA) { >> + Transaction Trans(TA); >> + assert(DiagI->getID() == diag::err_switch_into_protected_scope); >> + SourceLocation ErrLoc = DiagI->getLocation(); >> + bool handledAllNotes = true; >> + ++DiagI; >> + for (; DiagI != DiagE && DiagI->getLevel() == DiagnosticsEngine::Note; >> + ++DiagI) { >> + if (!handleProtectedNote(*DiagI, Cases, TA)) >> + handledAllNotes = false; >> + } >> + >> + if (handledAllNotes) >> + TA.clearDiagnostic(diag::err_switch_into_protected_scope, ErrLoc); >> +} >> + >> +void ProtectedScopeTraverser::traverseBody(BodyContext &BodyCtx) { >> + MigrationPass &Pass = BodyCtx.getMigrationContext().Pass; >> + SmallVector<CaseInfo, 16> Cases; >> + CaseCollector(Cases).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, Cases, Pass.TA); >> + continue; >> + } >> + ++I; >> + } >> +} >> >> Modified: cfe/trunk/lib/ARCMigrate/Transforms.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/ARCMigrate/Transforms.cpp?rev=171484&r1=171483&r2=171484&view=diff >> ============================================================================== >> --- cfe/trunk/lib/ARCMigrate/Transforms.cpp (original) >> +++ cfe/trunk/lib/ARCMigrate/Transforms.cpp Fri Jan 4 12:30:08 2013 >> @@ -564,6 +564,7 @@ >> } >> MigrateCtx.addTraverser(new PropertyRewriteTraverser()); >> MigrateCtx.addTraverser(new BlockObjCVariableTraverser()); >> + MigrateCtx.addTraverser(new ProtectedScopeTraverser()); >> >> MigrateCtx.traverse(pass.Ctx.getTranslationUnitDecl()); >> } >> >> Modified: cfe/trunk/lib/ARCMigrate/Transforms.h >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/ARCMigrate/Transforms.h?rev=171484&r1=171483&r2=171484&view=diff >> ============================================================================== >> --- cfe/trunk/lib/ARCMigrate/Transforms.h (original) >> +++ cfe/trunk/lib/ARCMigrate/Transforms.h Fri Jan 4 12:30:08 2013 >> @@ -135,6 +135,11 @@ >> virtual void traverseBody(BodyContext &BodyCtx); >> }; >> >> +class ProtectedScopeTraverser : public ASTTraverser { >> +public: >> + virtual void traverseBody(BodyContext &BodyCtx); >> +}; >> + >> // GC transformations >> >> class GCAttrsTraverser : public ASTTraverser { >> >> Modified: cfe/trunk/test/ARCMT/checking.m >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/ARCMT/checking.m?rev=171484&r1=171483&r2=171484&view=diff >> ============================================================================== >> --- cfe/trunk/test/ARCMT/checking.m (original) >> +++ cfe/trunk/test/ARCMT/checking.m Fri Jan 4 12:30:08 2013 >> @@ -178,13 +178,12 @@ >> } >> >> void test6(unsigned cond) { >> - // FIXME: Fix this automatically ? >> switch (cond) { >> case 0: >> ; >> - id x; // expected-note {{jump bypasses initialization of retaining >> variable}} >> + id x; >> >> - case 1: // expected-error {{switch case is in protected scope}} >> + case 1: >> break; >> } >> } >> @@ -293,10 +292,10 @@ >> void rdar9491791(int p) { >> switch (p) { >> case 3:; >> - NSObject *o = [[NSObject alloc] init]; // expected-note {{jump bypasses >> initialization of retaining variable}} >> + NSObject *o = [[NSObject alloc] init]; >> [o release]; >> break; >> - default: // expected-error {{switch case is in protected scope}} >> + default: >> break; >> } >> } >> >> Added: cfe/trunk/test/ARCMT/protected-scope.m >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/ARCMT/protected-scope.m?rev=171484&view=auto >> ============================================================================== >> --- cfe/trunk/test/ARCMT/protected-scope.m (added) >> +++ cfe/trunk/test/ARCMT/protected-scope.m Fri Jan 4 12:30:08 2013 >> @@ -0,0 +1,36 @@ >> +// RUN: %clang_cc1 -triple x86_64-apple-darwin10 -fsyntax-only -fobjc-arc >> -x objective-c %s.result >> +// RUN: arcmt-test --args -triple x86_64-apple-darwin10 -fsyntax-only -x >> objective-c %s > %t >> +// RUN: diff %t %s.result >> +// DISABLE: mingw32 >> + >> +#include "Common.h" >> + >> +void test(id p, int x) { >> + int v; >> + switch(x) { >> + case 0: >> + v++; >> + id w1 = p; >> + id w2 = p; >> + break; >> + case 1: >> + v++; >> + id w3 = p; >> + break; >> + case 2: >> + break; >> + default: >> + break; >> + } >> +} >> + >> +void test2(int p) { >> + switch (p) { >> + case 3:; >> + NSObject *o = [[NSObject alloc] init]; >> + [o release]; >> + break; >> + default: >> + break; >> + } >> +} >> >> Added: cfe/trunk/test/ARCMT/protected-scope.m.result >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/ARCMT/protected-scope.m.result?rev=171484&view=auto >> ============================================================================== >> --- cfe/trunk/test/ARCMT/protected-scope.m.result (added) >> +++ cfe/trunk/test/ARCMT/protected-scope.m.result Fri Jan 4 12:30:08 2013 >> @@ -0,0 +1,38 @@ >> +// RUN: %clang_cc1 -triple x86_64-apple-darwin10 -fsyntax-only -fobjc-arc >> -x objective-c %s.result >> +// RUN: arcmt-test --args -triple x86_64-apple-darwin10 -fsyntax-only -x >> objective-c %s > %t >> +// RUN: diff %t %s.result >> +// DISABLE: mingw32 >> + >> +#include "Common.h" >> + >> +void test(id p, int x) { >> + int v; >> + switch(x) { >> + case 0: { >> + v++; >> + id w1 = p; >> + id w2 = p; >> + break; >> + } >> + case 1: { >> + v++; >> + id w3 = p; >> + break; >> + } >> + case 2: >> + break; >> + default: >> + break; >> + } >> +} >> + >> +void test2(int p) { >> + switch (p) { >> + case 3: {; >> + NSObject *o = [[NSObject alloc] init]; >> + break; >> + } >> + default: >> + break; >> + } >> +} >> >> >> _______________________________________________ >> cfe-commits mailing list >> [email protected] >> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits > _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
