[PATCH] Clang Rename Tool

[Matthew Plant]

Questions before (hopefully) substantial cleanup

================
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> ...]"),
----------------
Manuel Klimek wrote:
> 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.
It's your decision, but let me also add that this will make writing shell scripts and such that use the command line tool.
Imagine a shell script that took every "the symbol at x:y:z could not be found, did you mean <other symbol>" and renamed the file accordingly.

================
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:
> 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.
It is useful, in some instance. because "new-name" can be any string, you could use it to do some pretty cool stuff I'm sure.
Also I don't have any problem maintaining just this tool, although I imagine there's just one maintainer for clang-tools-extra. It's not like I have work during the school year after all. 

================
Comment at: clang-rename/ClangRename.cpp:169
@@ +168,3 @@
+
+    if (PrevName.empty()) {
+      // Retrieve the previous name and USRs.
----------------
Manuel Klimek wrote:
> 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.
Yes, we do have to make sure we're looking at the file that has the USR first. That is guaranteed actually, because we push it to the front of the FileNames list.

================
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:
> 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).
That solves one problem, but there's another: 
What about the actual renaming part? I seem to recall a rewriter will become invalid when the RefactoringTool gets a new consumer. Does the factory need to create a new rewriter each time it creates a new consumer?

For what it's worth, I did try to use RefactoringTool; there's more examples on it. But for some reason during my experimentation I concluded that it was insufficient.

http://reviews.llvm.org/D4739