[PATCH] Clang Rename Tool
Manuel Klimek
klimek at google.com
Wed Aug 13 04:33:38 PDT 2014
I think there are basically 2 blockers before this can go in:
1. Make the tests actually test something :)
2. Use a member/getter for the result (see inline comment)
Otherwise LG for a first landing.
================
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>
+
----------------
Matthew Plant wrote:
> 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?
Not yet :) Patches welcome!
================
Comment at: clang-rename/ClangRename.cpp:141-145
@@ +140,7 @@
+ const auto &USRs = USRAction.getUSRs();
+ const auto &PrevName = USRAction.getUSRSpelling();
+
+ if (PrevName.empty())
+ // An error should have already been printed.
+ exit(1);
+
----------------
We really need to convert this to Diagnostics (fine in a follow-up patch though).
================
Comment at: clang-rename/USRFinder.cpp:41
@@ +40,3 @@
+ const SourceLocation Point,
+ const clang::NamedDecl **Result)
+ : Result(nullptr), SourceMgr(SourceMgr), LangOpts(LangOpts),
----------------
Use a class member and getter for the result.
================
Comment at: clang-rename/USRLocFinder.cpp:43-47
@@ +42,7 @@
+ if (getUSRForDecl(Decl) == USR) {
+ // Because we traverse the AST from top to bottom, it follows that the
+ // current locations must be further down (or right) than the previous one
+ // inspected. This has the effect of keeping LocationsFound sorted by
+ // line first column second with no effort of our own.
+ // This is correct as of 2014-06-27
+ LocationsFound.push_back(Decl->getLocation());
----------------
This somehow sneaked back in?
================
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() {
----------------
Matthew Plant wrote:
> Manuel Klimek wrote:
> > Thanks! This is much easier for me to read :)
> No problem, but boy was that offset hard to find :P
Ah, we need some adjustments:
1. Add // REQUIRES: shell at the top
2. copying the temp file only makes sense if we do in-place replacement; which I think we should do
3. The CHECK comments get matched by the CHECKS. You'll want:
sed 's,//.*,,' %t.cpp | FileCheck %s
4. Isn't that offset wrong? Both vim and
$ grep -FUbo 'foo;' /tmp/test
tell me the offset is 145. So, why not instead do
-offset=$(grep -FUbo 'foo;' %t.cpp |head -1 |cut -d: -f1)
================
Comment at: unittests/clang-rename/USRLocFindingTest.cpp:77
@@ +76,3 @@
+TEST(USRLocFinding, FindsVarUSR) {
+ const char VarTest[] = R"(
+namespace A {
----------------
I'm so so so sorry, but I lied about raw strings :'( - apparently VS 2012 doesn't support them yet, so they're not allowed in clang :( :( :(
http://reviews.llvm.org/D4739
More information about the cfe-commits
mailing list