[cfe-commits] r171484 - in /cfe/trunk: lib/ARCMigrate/ARCMT.cpp lib/ARCMigrate/Internals.h lib/ARCMigrate/TransProtectedScope.cpp lib/ARCMigrate/Transforms.cpp lib/ARCMigrate/Transforms.h test/ARCMT/checking.m test/ARCMT/protected-scope.m test/ARCMT/protected-scope.m.result
Argyrios Kyrtzidis
akyrtzi at gmail.com
Mon Jan 7 18:06:10 PST 2013
On Jan 4, 2013, at 6:06 PM, Jordan Rose <jordan_rose at apple.com> 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 <akyrtzi at gmail.com> 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
>> cfe-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
More information about the cfe-commits
mailing list