[clang-rename] exit code-related bugfix and code cleanup

[omtcyf0]

Summary

This patch does the following:

• enforces proper formatting for few files (i.e. deals with 80 linewidth violations and few other things)
• ensures '\n' chars are passed to the output streams instead of "\n" strings
• fixes a bug caused by calling cl::PrintHelpMessage(), which occasionally calls exit(0), so that exit(1) (which is right after cl::PrintHelpMessage line) becomes dead code

[vmiklos]

Can you please postpone the cleanup till http://reviews.llvm.org/D21814 is reviewed? The two patches conflict with each other, I fear. Thanks. :-)

[bkramer]

clang-rename/USRLocFinder.h

34

In LLVM we usually have only one space before a // comment. was this clang-formatted with google style?

[omtcyf0]

clang-rename/USRLocFinder.h

34

Oops. Yes, seems like my machine had Google as the default style, instead of the LLVM code style by default, which I usually have.

[vmiklos]

In D22091#476756, @vmiklos wrote:

Can you please postpone the cleanup till http://reviews.llvm.org/D21814 is reviewed? The two patches conflict with each other, I fear. Thanks. :-)

Ignore this, that one won't land as-is after all.

[alexfh]

clang-rename/USRLocFinder.cpp

63

This would look better as a range-based for loop.

73–75

clang::tooling:fixit::getText may be a good alternative here.

[omtcyf0]

clang-rename/USRLocFinder.cpp

73–76

Not sure whether I know how to address this one.

clang::tooling:fixit::getText returns text of a whole AST Node, while here we only want to get text of the first token that AST Node has. I.e. 'foo' vs. 'foo(0)'.

[omtcyf0]

Add new test, which ensures clang-rename failure if new name is not passed.

[omtcyf0]

Does it look good now?

[alexfh]

LG with one comment.

clang-rename/USRLocFinder.cpp

73–76

Fair enough. I didn't get the intent of the code then.

clang-rename/USRLocFinder.h

28

Use llvm style for names: USR, Decl.

[omtcyf0]

Thanks, @alexfh!

Can you please land it?

[alexfh]

In D22091#483834, @omtcyf0 wrote:

Thanks, @alexfh!

Can you please land it?

It's not easy to do from the phone ;) Please ask someone else (Ben?).

[bkramer]

on it ...

clang-rename/RenamingAction.cpp

public:
   }
 
   void HandleTranslationUnit(ASTContext &Context) override {
     const auto &SourceMgr = Context.getSourceManager();
     std::vector<SourceLocation> RenamingCandidates;
     std::vector<SourceLocation> NewCandidates;
 
     for (const auto &USR : USRs) {
-      NewCandidates = getLocationsOfUSR(USR, PrevName, Context.getTranslationUnitDecl());
+      NewCandidates = getLocationsOfUSR(USR, PrevName,
+                                        Context.getTranslationUnitDecl());
       RenamingCandidates.insert(RenamingCandidates.end(), NewCandidates.begin(),
                                 NewCandidates.end());
       NewCandidates.clear();
     }
 
     auto PrevNameLen = PrevName.length();
     if (PrintLocations)
       for (const auto &Loc : RenamingCandidates) {

clang-rename/USRLocFinder.h

 /// \brief Provides functionality for finding all instances of a USR in a given
 /// AST.
 ///
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_RENAME_USR_LOC_FINDER_H
 #define LLVM_CLANG_TOOLS_EXTRA_CLANG_RENAME_USR_LOC_FINDER_H
 
+#include "clang/AST/AST.h"
+#include "llvm/ADT/StringRef.h"
 #include <string>
 #include <vector>
 
-#include "llvm/ADT/StringRef.h"
-
 namespace clang {
-
-class Decl;
-class SourceLocation;
-
 namespace rename {
 
 // FIXME: make this an AST matcher. Wouldn't that be awesome??? I agree!
-std::vector<SourceLocation> getLocationsOfUSR(llvm::StringRef usr,
-                                              llvm::StringRef PrevName,
-                                              Decl *decl);
-}
-}
+std::vector<SourceLocation>
+getLocationsOfUSR(llvm::StringRef USR, llvm::StringRef PrevName, Decl *Decl);
+
+} // namespace rename
+} // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_RENAME_USR_LOC_FINDER_H

clang-rename/USRLocFinder.cpp

 namespace rename {
 
 namespace {
 // \brief This visitor recursively searches for all instances of a USR in a
 // translation unit and stores them for later usage.
 class USRLocFindingASTVisitor
     : public clang::RecursiveASTVisitor<USRLocFindingASTVisitor> {
 public:
-  explicit USRLocFindingASTVisitor(StringRef USR, StringRef PrevName) : USR(USR), PrevName(PrevName) {
-  }
+  explicit USRLocFindingASTVisitor(StringRef USR, StringRef PrevName)
+      : USR(USR), PrevName(PrevName) {}
 
   // Declaration visitors:
 
   bool VisitNamedDecl(const NamedDecl *Decl) {
     if (getUSRForDecl(Decl) == USR) {
       LocationsFound.push_back(Decl->getLocation());
     }
     return true;
   }
 
   bool VisitVarDecl(clang::VarDecl *Decl) {
     clang::QualType Type = Decl->getType();
     const clang::RecordDecl *RecordDecl = Type->getPointeeCXXRecordDecl();
     if (RecordDecl) {
       if (getUSRForDecl(RecordDecl) == USR) {
         // The declaration refers to a type that is to be renamed.
         LocationsFound.push_back(Decl->getTypeSpecStartLoc());
       }
     }
     return true;
   }
 
   bool VisitCXXConstructorDecl(clang::CXXConstructorDecl *ConstructorDecl) {
     const ASTContext &Context = ConstructorDecl->getASTContext();
-    for (clang::CXXConstructorDecl::init_const_iterator it = ConstructorDecl->init_begin(); it != ConstructorDecl->init_end(); ++it) {
-      const clang::CXXCtorInitializer* Initializer = *it;
+    for (auto &Initializer : ConstructorDecl->inits()) {
       if (Initializer->getSourceOrder() == -1) {
         // Ignore implicit initializers.
         continue;
       }
 
       if (const clang::FieldDecl *FieldDecl = Initializer->getAnyMember()) {
         if (getUSRForDecl(FieldDecl) == USR) {
           // The initializer refers to a field that is to be renamed.
           SourceLocation Location = Initializer->getSourceLocation();
-          StringRef TokenName = Lexer::getSourceText(CharSourceRange::getTokenRange(Location), Context.getSourceManager(), Context.getLangOpts());
+          StringRef TokenName = Lexer::getSourceText(
+              CharSourceRange::getTokenRange(Location),
+              Context.getSourceManager(), Context.getLangOpts());
           if (TokenName == PrevName) {
-            // The token of the source location we find actually has the old name.
+            // The token of the source location we find actually has the old
+            // name.
             LocationsFound.push_back(Initializer->getSourceLocation());
           }
         }
       }
     }
 
     if (getUSRForDecl(ConstructorDecl) == USR) {
       // This takes care of the class name part of a non-inline ctor definition.
while (NameLoc) {
         LocationsFound.push_back(NameLoc.getLocalBeginLoc());
       NameLoc = NameLoc.getPrefix();
     }
   }
 
   bool handleCXXNamedCastExpr(clang::CXXNamedCastExpr *Expr) {
     clang::QualType Type = Expr->getType();
     // See if this a cast of a pointer.
-    const RecordDecl* Decl = Type->getPointeeCXXRecordDecl();
+    const RecordDecl *Decl = Type->getPointeeCXXRecordDecl();
     if (!Decl) {
       // See if this is a cast of a reference.
       Decl = Type->getAsCXXRecordDecl();
     }
 
     if (Decl && getUSRForDecl(Decl) == USR) {
-      SourceLocation Location = Expr->getTypeInfoAsWritten()->getTypeLoc().getBeginLoc();
+      SourceLocation Location =
+          Expr->getTypeInfoAsWritten()->getTypeLoc().getBeginLoc();
       LocationsFound.push_back(Location);
     }
 
     return true;
   }
 
   // All the locations of the USR were found.
   const std::string USR;
   // Old name that is renamed.
   const std::string PrevName;
   std::vector<clang::SourceLocation> LocationsFound;
 };
 } // namespace
 
-std::vector<SourceLocation> getLocationsOfUSR(StringRef USR,
-                                              StringRef PrevName,
+std::vector<SourceLocation> getLocationsOfUSR(StringRef USR, StringRef PrevName,
                                               Decl *Decl) {
   USRLocFindingASTVisitor visitor(USR, PrevName);
 
   visitor.TraverseDecl(Decl);
   return visitor.getLocationsFound();
 }
 
 } // namespace rename
 } // namespace clang

clang-rename/tool/ClangRename.cpp

 #include "clang/Parse/Parser.h"
 #include "clang/Rewrite/Core/Rewriter.h"
 #include "clang/Tooling/CommonOptionsParser.h"
 #include "clang/Tooling/Refactoring.h"
 #include "clang/Tooling/ReplacementsYaml.h"
 #include "clang/Tooling/Tooling.h"
 #include "llvm/ADT/IntrusiveRefCntPtr.h"
 #include "llvm/Support/Host.h"
-#include <cstdlib>
 #include <string>
 
 using namespace llvm;
 
 cl::OptionCategory ClangRenameCategory("Clang-rename options");
 
 static cl::opt<std::string>
 NewName(
ExportFixes(
     "export-fixes",
     cl::desc("YAML file to store suggested fixes in."),
     cl::value_desc("filename"),
     cl::cat(ClangRenameCategory));
 
 #define CLANG_RENAME_VERSION "0.0.1"
 
 static void PrintVersion() {
-  outs() << "clang-rename version " << CLANG_RENAME_VERSION << "\n";
+  outs() << "clang-rename version " << CLANG_RENAME_VERSION << '\n';
 }
 
 using namespace clang;
 
 const char RenameUsage[] = "A tool to rename symbols in C/C++ code.\n\
 clang-rename renames every occurrence of a symbol found at <offset> in\n\
 <source0>. If -i is specified, the edited files are overwritten to disk.\n\
 Otherwise, the results are written to stdout.\n";
 
 int main(int argc, const char **argv) {
   cl::SetVersionPrinter(PrintVersion);
   tooling::CommonOptionsParser OP(argc, argv, ClangRenameCategory, RenameUsage);
 
   // Check the arguments for correctness.
 
   if (NewName.empty()) {
     errs() << "clang-rename: no new name provided.\n\n";
-    cl::PrintHelpMessage();
     exit(1);
   }
 
   // Get the USRs.
   auto Files = OP.getSourcePathList();
   tooling::RefactoringTool Tool(OP.getCompilations(), Files);
   rename::USRFindingAction USRAction(SymbolOffset, OldName);
 
   // Find the USRs.
   Tool.run(tooling::newFrontendActionFactory(&USRAction).get());
   const auto &USRs = USRAction.getUSRs();
   const auto &PrevName = USRAction.getUSRSpelling();
 
-  if (PrevName.empty())
+  if (PrevName.empty()) {
     // An error should have already been printed.
     exit(1);
+  }
 
-  if (PrintName)
-    errs() << "clang-rename: found name: " << PrevName << "\n";
+  if (PrintName) {
+    errs() << "clang-rename: found name: " << PrevName << '\n';
+  }
 
   // Perform the renaming.
   rename::RenamingAction RenameAction(NewName, PrevName, USRs,
                                       Tool.getReplacements(), PrintLocations);
   auto Factory = tooling::newFrontendActionFactory(&RenameAction);
   int res;
 
   if (Inplace) {

test/clang-rename/NoNewName.cpp

+// This test is a copy of ConstCastExpr.cpp with a single change:
+// -new-name hasn't been passed to clang-rename, so this test should give an
+// error.
+// RUN: not clang-rename -offset=133 %s 2>&1 | FileCheck %s
+// CHECK: clang-rename: no new name provided.
+
+class Cla {
+public:
+  int getValue() {
+    return 0;
+  }
+};
+
+int main() {
+  const Cla *C = new Cla();
+  const_cast<Cla *>(C)->getValue();
+}
+
+// Use grep -FUbo 'Cla' <file> to get the correct offset of foo when changing
+// this file.