================
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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
