[PATCH] Clang Rename Tool

[Matthew Plant]

================
Comment at: clang-rename/ClangRename.cpp:69-71
@@ +68,5 @@
+            cl::cat(ClangRenameCategory));
+static cl::opt<std::string> SymbolLocation(cl::Positional,
+                                           cl::desc("<file>:<line>:<column>"),
+                                           cl::cat(ClangRenameCategory));
+static cl::list<std::string> FileNames(cl::Positional, cl::desc("[<file> ...]"),
----------------
Manuel Klimek wrote:
> I usually find offsets better to use for tools (and humans are not going to enter lines and columns here anyway).
I added the option to use an offset. And I would not be too sure about the second claim. No matter how obtuse an interface, someone will prefer it. After all, ed is bundled with every Ubuntu install ;)

================
Comment at: clang-rename/ClangRename.cpp:82
@@ +81,3 @@
+      (FileName == "-" ? "<stdin>" : FileName), Source->getBufferSize(), 0);
+  Sources.overrideFileContents(Entry, Source, true);
+  return Sources.createFileID(Entry, SourceLocation(), SrcMgr::C_User);
----------------
Manuel Klimek wrote:
> This is also already a feature of ClangTool / RefactoringTool if I'm not mistaken.
You are correct. As explained later, we can't use RefactoringTool, unless you can think of a solution to the problem explained later.

================
Comment at: clang-rename/ClangRename.cpp:90
@@ +89,3 @@
+// the input was given by a Clang getNameAsString method.
+static std::string getOpCallShortForm(const std::string &Call) {
+  const std::string Prefix = "operator";
----------------
Manuel Klimek wrote:
> I'd try to not use getNameAsString to do matching against - can't we solve this in the type system?
Not in a way that also conveniently gives us the operator spelling. 

================
Comment at: clang-rename/ClangRename.cpp:100
@@ +99,3 @@
+// Get the USRs for the constructors of the class.
+static std::vector<std::string> getClassEquivUSRs(const CXXRecordDecl *Decl) {
+  std::vector<std::string> USRs;
----------------
Manuel Klimek wrote:
> Name this getAllConstructorUSRs?
I was thinking that there would be other get*EquivUSRs functions. The term "Equivalent USRs" makes more sense to me in a context where there are multiple of them. But that is not the case here, so I'll change it.

================
Comment at: clang-rename/ClangRename.cpp:113
@@ +112,3 @@
+  // explicit calls to a destructor through TagTypeLoc (and it is better for the
+  // purpose of renaming).
+  //
----------------
Manuel Klimek wrote:
> I wonder whether we could do something similar for constructors (perhaps by changing the AST).
I'm not so sure. Constructors and destructors are pretty different; there can only be one destructor for example.

================
Comment at: clang-rename/ClangRename.cpp:131-133
@@ +130,5 @@
+  for (auto Loc : OldLocs) {
+    // If the current location poihnts to the "operator" keyword, we want to
+    // skip to the actual operator's spelling. We don't try to do this very; if
+    // the operator isn't the next token over, we give up on the location.
+    SmallVector<char, 32> Buffer;
----------------
Manuel Klimek wrote:
> Manuel Klimek wrote:
> > points
> Renaming operators seems ... strange; I would expect to add that (if at all) somewhen much later.
It is very strange, but it's there. The code for fixOperatorLocs however is not too complicated, and we already have some code for operator renaming I'm planning to place in USR(Loc)Finder later. So I'd rather not remove this just for the sake of the review.

================
Comment at: clang-rename/ClangRename.cpp:169
@@ +168,3 @@
+
+    if (PrevName.empty()) {
+      // Retrieve the previous name and USRs.
----------------
Manuel Klimek wrote:
> I find this rather strange architecturally - why don't we just visit the whole thing twice?
The reason is that the point may not be in the file we are currently looking at. If we were to attempt to find the USR, we would not find it. Thus we must save it when we initially find it. 

this is part of the reason why we can't use refactoring tool - see below for more info on that.

================
Comment at: clang-rename/ClangRename.cpp:233
@@ +232,3 @@
+
+    // ...and we're done!
+  }
----------------
Manuel Klimek wrote:
> Remove comment.
But then how will anyone know what the '}' means?? (kidding)

================
Comment at: clang-rename/ClangRename.cpp:248-258
@@ +247,13 @@
+static bool Rename() {
+  CompilerInstance Compiler;
+
+  auto *Invocation = new CompilerInvocation;
+  Compiler.setInvocation(Invocation);
+
+  // Make sure we're parsing the right language.
+  Compiler.getLangOpts().CPlusPlus11 = true;
+  Compiler.getLangOpts().GNUKeywords = true;
+  Compiler.getLangOpts().NoBuiltin = false;
+  Invocation->setLangDefaults(Compiler.getLangOpts(), IK_CXX,
+                              LangStandard::lang_gnucxx11);
+
----------------
Manuel Klimek wrote:
> Any reason you're not using RefactoringTool (which already does basically everything that's in this file)?
We need to create a factory for RefactoringTool, which allocates and returns a new consumer per file. Unfortunately, we can't in the factory control where the allocated consumer is deleted - it's free'd by RefactoringTool! This means we can't have persistent state in our consumer without global or static variables - which would require us to violate  http://llvm.org/docs/CodingStandards.html#do-not-use-static-constructors. Since we need persistent state for the USR (see comment above), we are left with avoiding RefactoringTool

http://reviews.llvm.org/D4739