[arcmt] Fix test/ARCMT/remove-statements.m regression due to

Objective-C method buffering(rdar://10056942)

Turned out the same issue existed for C++ inline methods.

llvm-svn: 138960

clang/include/clang/Basic/SourceManager.h

@@ -1116,6 +1116,19 @@ public:
   /// \returns true if LHS source location comes before RHS, false otherwise.
   bool isBeforeInTranslationUnit(SourceLocation LHS, SourceLocation RHS) const;
 
+  /// \brief Comparison function class.
+  class LocBeforeThanCompare : public std::binary_function<SourceLocation,
+                                                         SourceLocation, bool> {
+    SourceManager &SM;
+
+  public:
+    explicit LocBeforeThanCompare(SourceManager &SM) : SM(SM) { }
+
+    bool operator()(SourceLocation LHS, SourceLocation RHS) const {
+      return SM.isBeforeInTranslationUnit(LHS, RHS);
+    }
+  };
+
   /// \brief Determines the order of 2 source locations in the "source location
   /// address space".
   bool isBeforeInSLocAddrSpace(SourceLocation LHS, SourceLocation RHS) const {

clang/lib/ARCMigrate/TransEmptyStatementsAndDealloc.cpp

@@ -22,25 +22,68 @@
 #include "Transforms.h"
 #include "Internals.h"
 #include "clang/AST/StmtVisitor.h"
+#include "clang/Basic/SourceManager.h"
 
 using namespace clang;
 using namespace arcmt;
 using namespace trans;
 
+static bool isEmptyARCMTMacroStatement(NullStmt *S,
+                                       std::vector<SourceLocation> &MacroLocs,
+                                       ASTContext &Ctx) {
+  if (!S->hasLeadingEmptyMacro())
+    return false;
+
+  SourceLocation SemiLoc = S->getSemiLoc();
+  if (SemiLoc.isInvalid() || SemiLoc.isMacroID())
+    return false;
+
+  if (MacroLocs.empty())
+    return false;
+
+  SourceManager &SM = Ctx.getSourceManager();
+  std::vector<SourceLocation>::iterator
+    I = std::upper_bound(MacroLocs.begin(), MacroLocs.end(), SemiLoc,
+                         SourceManager::LocBeforeThanCompare(SM));
+  --I;
+  SourceLocation
+      AfterMacroLoc = I->getFileLocWithOffset(getARCMTMacroName().size());
+  assert(AfterMacroLoc.isFileID());
+
+  if (AfterMacroLoc == SemiLoc)
+    return true;
+
+  int RelOffs = 0;
+  if (!SM.isInSameSLocAddrSpace(AfterMacroLoc, SemiLoc, &RelOffs))
+    return false;
+  if (RelOffs < 0)
+    return false;
+
+  // We make the reasonable assumption that a semicolon after 100 characters
+  // means that it is not the next token after our macro. If this assumption
+  // fails it is not critical, we will just fail to clear out, e.g., an empty
+  // 'if'.
+  if (RelOffs - getARCMTMacroName().size() > 100)
+    return false;
+
+  SourceLocation AfterMacroSemiLoc = findSemiAfterLocation(AfterMacroLoc, Ctx);
+  return AfterMacroSemiLoc == SemiLoc;
+}
+
 namespace {
 
 /// \brief Returns true if the statement became empty due to previous
 /// transformations.
 class EmptyChecker : public StmtVisitor<EmptyChecker, bool> {
   ASTContext &Ctx;
-  llvm::DenseSet<unsigned> &MacroLocs;
+  std::vector<SourceLocation> &MacroLocs;
 
 public:
-  EmptyChecker(ASTContext &ctx, llvm::DenseSet<unsigned> &macroLocs)
+  EmptyChecker(ASTContext &ctx, std::vector<SourceLocation> &macroLocs)
     : Ctx(ctx), MacroLocs(macroLocs) { }
 
   bool VisitNullStmt(NullStmt *S) {
-    return isMacroLoc(S->getLeadingEmptyMacroLoc());
+    return isEmptyARCMTMacroStatement(S, MacroLocs, Ctx);
   }
   bool VisitCompoundStmt(CompoundStmt *S) {
     if (S->body_empty())
@@ -102,23 +145,14 @@ public:
       return false;
     return Visit(S->getSubStmt());
   }
-
-private:
-  bool isMacroLoc(SourceLocation loc) {
-    if (loc.isInvalid()) return false;
-    return MacroLocs.count(loc.getRawEncoding());
-  }
 };
 
 class EmptyStatementsRemover :
                             public RecursiveASTVisitor<EmptyStatementsRemover> {
   MigrationPass &Pass;
-  llvm::DenseSet<unsigned> &MacroLocs;
 
 public:
-  EmptyStatementsRemover(MigrationPass &pass,
+  EmptyStatementsRemover(MigrationPass &pass) : Pass(pass) { }
-                         llvm::DenseSet<unsigned> &macroLocs)
-    : Pass(pass), MacroLocs(macroLocs) { }
 
   bool TraverseStmtExpr(StmtExpr *E) {
     CompoundStmt *S = E->getSubStmt();
@@ -138,17 +172,12 @@ public:
     return true;
   }
 
-  bool isMacroLoc(SourceLocation loc) {
-    if (loc.isInvalid()) return false;
-    return MacroLocs.count(loc.getRawEncoding());
-  }
-
   ASTContext &getContext() { return Pass.Ctx; }
 
 private:
   void check(Stmt *S) {
     if (!S) return;
-    if (EmptyChecker(Pass.Ctx, MacroLocs).Visit(S)) {
+    if (EmptyChecker(Pass.Ctx, Pass.ARCMTMacroLocs).Visit(S)) {
       Transaction Trans(Pass.TA);
       Pass.TA.removeStmt(S);
     }
@@ -157,8 +186,8 @@ private:
 
 } // anonymous namespace
 
-static bool isBodyEmpty(CompoundStmt *body,
+static bool isBodyEmpty(CompoundStmt *body, ASTContext &Ctx,
-                        ASTContext &Ctx, llvm::DenseSet<unsigned> &MacroLocs) {
+                        std::vector<SourceLocation> &MacroLocs) {
   for (CompoundStmt::body_iterator
          I = body->body_begin(), E = body->body_end(); I != E; ++I)
     if (!EmptyChecker(Ctx, MacroLocs).Visit(*I))
@@ -167,8 +196,7 @@ static bool isBodyEmpty(CompoundStmt *body,
   return true;
 }
 
-static void removeDeallocMethod(MigrationPass &pass,
+static void removeDeallocMethod(MigrationPass &pass) {
-                                llvm::DenseSet<unsigned> &MacroLocs) {
   ASTContext &Ctx = pass.Ctx;
   TransformActions &TA = pass.TA;
   DeclContext *DC = Ctx.getTranslationUnitDecl();
@@ -183,7 +211,7 @@ static void removeDeallocMethod(MigrationPass &pass,
       ObjCMethodDecl *MD = *MI;
       if (MD->getMethodFamily() == OMF_dealloc) {
         if (MD->hasBody() &&
-            isBodyEmpty(MD->getCompoundBody(), Ctx, MacroLocs)) {
+            isBodyEmpty(MD->getCompoundBody(), Ctx, pass.ARCMTMacroLocs)) {
           Transaction Trans(TA);
           TA.remove(MD->getSourceRange());
         }
@@ -194,14 +222,9 @@ static void removeDeallocMethod(MigrationPass &pass,
 }
 
 void trans::removeEmptyStatementsAndDealloc(MigrationPass &pass) {
-  llvm::DenseSet<unsigned> MacroLocs;
+  EmptyStatementsRemover(pass).TraverseDecl(pass.Ctx.getTranslationUnitDecl());
-  for (unsigned i = 0, e = pass.ARCMTMacroLocs.size(); i != e; ++i)
-    MacroLocs.insert(pass.ARCMTMacroLocs[i].getRawEncoding());
 
-  EmptyStatementsRemover(pass, MacroLocs)
+  removeDeallocMethod(pass);
-    .TraverseDecl(pass.Ctx.getTranslationUnitDecl());
-
-  removeDeallocMethod(pass, MacroLocs);
 
   for (unsigned i = 0, e = pass.ARCMTMacroLocs.size(); i != e; ++i) {
     Transaction Trans(pass.TA);

clang/lib/ARCMigrate/Transforms.cpp

@@ -91,6 +91,18 @@ bool trans::canApplyWeak(ASTContext &Ctx, QualType type) {
 /// source location will be invalid.
 SourceLocation trans::findLocationAfterSemi(SourceLocation loc,
                                             ASTContext &Ctx) {
+  SourceLocation SemiLoc = findSemiAfterLocation(loc, Ctx);
+  if (SemiLoc.isInvalid())
+    return SourceLocation();
+  return SemiLoc.getFileLocWithOffset(1);
+}
+
+/// \brief \arg Loc is the end of a statement range. This returns the location
+/// of the semicolon following the statement.
+/// If no semicolon is found or the location is inside a macro, the returned
+/// source location will be invalid.
+SourceLocation trans::findSemiAfterLocation(SourceLocation loc,
+                                            ASTContext &Ctx) {
   SourceManager &SM = Ctx.getSourceManager();
   if (loc.isMacroID()) {
     if (!Lexer::isAtEndOfMacroExpansion(loc, SM, Ctx.getLangOptions()))
@@ -119,7 +131,7 @@ SourceLocation trans::findLocationAfterSemi(SourceLocation loc,
   if (tok.isNot(tok::semi))
     return SourceLocation();
 
-  return tok.getLocation().getFileLocWithOffset(1);
+  return tok.getLocation();
 }
 
 bool trans::hasSideEffects(Expr *E, ASTContext &Ctx) {

clang/lib/ARCMigrate/Transforms.h

@@ -54,6 +54,12 @@ bool canApplyWeak(ASTContext &Ctx, QualType type);
 /// source location will be invalid.
 SourceLocation findLocationAfterSemi(SourceLocation loc, ASTContext &Ctx);
 
+/// \brief \arg Loc is the end of a statement range. This returns the location
+/// of the semicolon following the statement.
+/// If no semicolon is found or the location is inside a macro, the returned
+/// source location will be invalid.
+SourceLocation findSemiAfterLocation(SourceLocation loc, ASTContext &Ctx);
+
 bool hasSideEffects(Expr *E, ASTContext &Ctx);
 bool isGlobalVar(Expr *E);
 /// \brief Returns "nil" or "0" if 'nil' macro is not actually defined.

clang/test/ARCMT/cxx-rewrite.mm

@@ -14,6 +14,8 @@ struct foo {
         NSAutoreleasePool *pool = [NSAutoreleasePool new];
         [[[NSString string] retain] release];
         [pool drain];
+        if (s)
+          [s release];
     }
     ~foo(){ [s release]; }
 private:

clang/test/ARCMT/remove-statements.m

@@ -1,7 +1,6 @@
 // RUN: %clang_cc1 -triple x86_64-apple-darwin10 -fobjc-nonfragile-abi -fsyntax-only -fobjc-arc -x objective-c %s.result
 // RUN: arcmt-test --args -triple x86_64-apple-darwin10 -fobjc-nonfragile-abi -fsyntax-only -x objective-c %s > %t
 // RUN: diff %t %s.result
-// XFAIL: *
 
 #include "Common.h"