[PATCH] Clang Rename Tool

[Manuel Klimek]

================
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>
+
----------------
Are all of these still needed?

================
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;
+
----------------
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.

================
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
----------------
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? :)

================
Comment at: clang-rename/USRFindingAction.h:32
@@ +31,3 @@
+
+  std::string PrevName;
+  std::vector<std::string> USRs;
----------------
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).

================
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() {
----------------
Thanks! This is much easier for me to read :)

================
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]);
+  }
----------------
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?

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

================
Comment at: unittests/clang-rename/USRLocFindingTest.cpp:111
@@ +110,3 @@
+}
+
+
----------------
Nit: remove redundant empty line.

http://reviews.llvm.org/D4739