[PATCH] Clang Rename Tool

[Manuel Klimek]

================
Comment at: clang-rename/ClangRename.cpp:66-75
@@ +65,12 @@
+                                       cl::cat(ClangRenameCategory));
+static cl::opt<std::string>
+    NewName("new-name", cl::desc("The new name to change the symbol to."),
+            cl::cat(ClangRenameCategory));
+static cl::opt<std::string> SymbolLocation(cl::Positional,
+                                           cl::desc("<file>:<line>:<column>"),
+                                           cl::cat(ClangRenameCategory));
+static cl::opt<unsigned> SymbolOffset(
+    "offset",
+    cl::desc("Locates the symbol by offset as opposed to <line>:<column>."),
+    cl::cat(ClangRenameCategory));
+static cl::list<std::string> FileNames(cl::Positional, cl::desc("[<file> ...]"),
----------------
Ok, I'm *very* opposed to having both interfaces. This just complicates the code. I stand by what I said - I think users will not enter this manually (because that doesn't really make sense), so we should only use the offset.

================
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";
----------------
Matthew Plant wrote:
> 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. 
I thought there was a different way to get the operator name... I'll take a look.

================
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;
----------------
Matthew Plant wrote:
> 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.
"It's there" is not a good argument. Neither is "it's not too complicated".
A good argument for a feature needs to involve "it's useful". Note that the code will be maintained by other people, which is why we have pretty strict requirements on the code.

================
Comment at: clang-rename/ClangRename.cpp:169
@@ +168,3 @@
+
+    if (PrevName.empty()) {
+      // Retrieve the previous name and USRs.
----------------
Matthew Plant wrote:
> 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.
I'm not sure I understand:
If you're concerned about finding it when we want to look at multiple files, we have to make sure we look at the file that has the USR *first*, otherwise we might miss refactorings, right?
If you're only concerned about a single file, visiting it twice won't hurt, and be architecturally the right thing to do so we don't run into problems later.

================
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);
+
----------------
Matthew Plant wrote:
> 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 
"This means we can't have persistent state in our consumer without global or static variables"

Just have the state at function scope (for example, here, in a class) and pass a pointer to that state down through the factory into the consumer. The consumer doesn't need to delete things it references when it's destroyed (or I'm missing something).

http://reviews.llvm.org/D4739