[PATCH] Clang Rename Tool

[Matthew Plant]

================
Comment at: clang-rename/ClangRename.cpp:16-42
@@ +15,29 @@
+
+#include "USRFindingAction.h"
+#include "RenamingAction.h"
+#include "clang/AST/ASTConsumer.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/Basic/FileManager.h"
+#include "clang/Basic/LangOptions.h"
+#include "clang/Basic/TargetInfo.h"
+#include "clang/Basic/TargetOptions.h"
+#include "clang/Frontend/CommandLineSourceLoc.h"
+#include "clang/Frontend/CompilerInstance.h"
+#include "clang/Frontend/FrontendAction.h"
+#include "clang/Frontend/TextDiagnosticPrinter.h"
+#include "clang/Lex/Preprocessor.h"
+#include "clang/Lex/Lexer.h"
+#include "clang/Parse/Parser.h"
+#include "clang/Parse/ParseAST.h"
+#include "clang/Rewrite/Core/Rewriter.h"
+#include "clang/Tooling/CommonOptionsParser.h"
+#include "clang/Tooling/Refactoring.h"
+#include "clang/Tooling/Tooling.h"
+#include "llvm/ADT/IntrusiveRefCntPtr.h"
+#include "llvm/Support/Host.h"
+#include <stdio.h>
+#include <stdlib.h>
+#include <ctype.h>
+#include <string>
+#include <vector>
+
----------------
Manuel Klimek wrote:
> Are all of these still needed?
Not entirely sure. The only real way to determine is to remove one at a time, or manually go through and determine if each is actually used.
Isn't there a clang tool for this? remove-unused-headers or something?

================
Comment at: clang-rename/RenamingAction.cpp:34-41
@@ +33,10 @@
+using namespace llvm;
+
+extern cl::OptionCategory ClangRenameCategory;
+
+static cl::opt<bool> PrintLocations(
+    "pl", cl::desc("Print the locations affected by renaming to stderr."),
+    cl::cat(ClangRenameCategory));
+
+extern cl::opt<std::string> NewName;
+
----------------
Manuel Klimek wrote:
> Generally, we shouldn't rely on global variables to communicate between classes. The solution is to put all flags into the main file, and pass the information into the relevant classes via parameters.
Right-o, will fix. Thought it was a little dubious but decided to do it anyway. 

================
Comment at: clang-rename/USRFinder.cpp:56-57
@@ +55,4 @@
+    const auto *FuncDecl = dyn_cast<FunctionDecl>(Decl);
+    if (FuncDecl && (FuncDecl->isOverloadedOperator() ||
+                      isa<CXXConversionDecl>(FuncDecl))) {
+      // Since overloaded operator declarations consist of multiple tokens, we
----------------
Manuel Klimek wrote:
> You removed the operator stuff in the other file, but here it's still in. Is that intentional? Will tests break if we remove it? :)
Not intentional; I'll remove it.

================
Comment at: clang-rename/USRFindingAction.h:32
@@ +31,3 @@
+
+  std::string PrevName;
+  std::vector<std::string> USRs;
----------------
Manuel Klimek wrote:
> I don't think "Prev" is a good name part here... (I assume this is left from when this was strictly for renaming).
> 
> Also, I'd prefer to have private variables and getters here (otherwise it's unclear what is to be set from where).
Hmmm, sure. I renamed PrevName to SpellingName, I think that's better. Also added the setters and getters.

================
Comment at: test/clang-rename/VarTest.cpp:3-8
@@ +2,8 @@
+// RUN: clang-rename -offset=126 -new-name=hector %t.cpp -- | FileCheck %s < %t.cpp
+namespace A {
+int foo;  // CHECK: int hector;
+}
+int foo;  // CHECK: int foo;
+int bar = foo; // CHECK: bar = foo;
+int baz = A::foo; // CHECK: baz = A::hector;
+void fun1() {
----------------
Manuel Klimek wrote:
> Thanks! This is much easier for me to read :)
No problem, but boy was that offset hard to find :P

================
Comment at: unittests/clang-rename/USRLocFindingTest.cpp:61-66
@@ +60,8 @@
+    EXPECT_EQ(1u, Action.USRs.size());
+    if (USRMap.find(Group.group) == USRMap.end()) {
+      NumGroups++;
+      USRMap[Group.group] = Action.USRs[0];
+      AllUSRs.insert(Action.USRs[0]);
+    } else
+      EXPECT_EQ(USRMap[Group.group], Action.USRs[0]);
+  }
----------------
Manuel Klimek wrote:
> I'm not 100% sure I understand what the branch is for here (and the function is lacking comments ;)
> It looks like it's basically saying "those offsets should yield the same USR", right?
> I think that's a good test idea (as it keeps the USR as an implementation detail, which is nice), but why not use a different data structure here, like:
> vector<vector<unsigned>> offsets;
> where offsets[group] is a list of offsets that should evaluate to the same USR?
You're correct on the intent. The idea is that we don't want to rely on USRs being any particular value so long as that the invariants described in rename_test.py are held. I'm not sure why I didn't choose a vector of vectors, that makes a lot more sense actually. I seem to recall the thought entering my brain but being dismissed for some reason. To be fixed. 

================
Comment at: unittests/clang-rename/USRLocFindingTest.cpp:74
@@ +73,3 @@
+TEST(USRLocFinding, FindsVarUSR) {
+  const char VarTest[] = "\
+namespace A {\n\
----------------
Manuel Klimek wrote:
> You can use raw strings to make this look better...
Ah yes

http://reviews.llvm.org/D4739