[PATCH] Clang Rename Tool
Manuel Klimek
klimek at google.com
Mon Aug 4 03:22:54 PDT 2014
================
Comment at: clang-rename/ClangRename.cpp:159-188
@@ +158,32 @@
+
+ if (PrevName.empty()) {
+ // Retrieve the previous name and USRs.
+ // The source file we are searching will always be the main source file.
+ const auto Point = SourceMgr.getLocForStartOfFile(
+ SourceMgr.getMainFileID()).getLocWithOffset(SymbolOffset);
+ const NamedDecl *TargetDecl = getNamedDeclAt(Context, Point);
+ if (TargetDecl == nullptr) {
+ errs() << "clang-rename: no symbol found at given location\n";
+ *Failed = true;
+ return;
+ }
+
+ // If the decl is in any way related to a class, we want to make sure we
+ // search for the constructor and destructor as well as everything else.
+ if (const auto *CtorDecl = dyn_cast<CXXConstructorDecl>(TargetDecl))
+ TargetDecl = CtorDecl->getParent();
+ else if (const auto *DtorDecl = dyn_cast<CXXDestructorDecl>(TargetDecl))
+ TargetDecl = DtorDecl->getParent();
+
+ if (const auto *Record = dyn_cast<CXXRecordDecl>(TargetDecl)) {
+ auto ExtraUSRs = getAllConstructorUSRs(Record);
+ USRs.insert(USRs.end(), ExtraUSRs.begin(), ExtraUSRs.end());
+ }
+
+ // Get the primary USR and previous symbol's name.
+ USRs.push_back(getUSRForDecl(TargetDecl));
+ PrevName = TargetDecl->getNameAsString();
+ if (PrintName)
+ errs() << "clang-rename: found symbol: " << PrevName << "\n";
+ }
+
----------------
As mentioned before, I think we want an extra run for this, so it can be called outside the RenamingASTConsumer. It seems completely unrelated to renaming.
================
Comment at: clang-rename/ClangRename.cpp:201-206
@@ +200,8 @@
+
+ // If we're renaming an operator, we need to fix some of our source
+ // locations.
+ auto OpShortForm = getOpCallShortForm(PrevName);
+ if (!OpShortForm.empty())
+ RenamingCandidates =
+ fixOperatorLocs(OpShortForm, RenamingCandidates, SourceMgr, LangOpts);
+
----------------
What's the reason not to sink this into getLocationsOfUSR?
================
Comment at: clang-rename/ClangRename.cpp:215-216
@@ +214,4 @@
+ << FullLoc.getSpellingColumnNumber() << "\n";
+ Replaces.insert(tooling::Replacement(SourceMgr, Loc, PrevNameLen,
+ NewName));
+ }
----------------
I'd pull that out of the loop and invert the if (PrintLocations) to only affect the errs() <<. Also, I'd probably use outs() unless this is for debugging, in which case we'd want to use the debugging facilities instead of just dumping to stderr...
================
Comment at: clang-rename/ClangRename.cpp:262
@@ +261,3 @@
+
+ class RenamingFrontendAction : public ASTFrontendAction {
+ public:
----------------
I'd prefer this class (and everything it uses) to go into its own .h/.cpp file.
Perhaps call it RenamingAction.h/.cpp.
I'd still want to modularize it further:
- One action to find the USRs (perhaps just export it from USRFinder.h)
- (mid-term) An AST matcher matchUSRs(const std::vector<std::string>&) that matches when a node matches one of the given USRs; this is non-trival to implement, so I'm fine with having an extra AST consumer for now (the getLocationsOfUSRs seems like a good abstraction for now, but I'd want to add a FIXME)
- most of the code in RenamingASTConsumer seems non-renaming related; see comments there.
================
Comment at: clang-rename/ClangRename.cpp:274
@@ +273,3 @@
+
+ bool hasFailed() { return FailureCond; }
+
----------------
Please add:
// FIXME: Use clang diagnostics.
================
Comment at: clang-rename/ClangRename.cpp:283
@@ +282,3 @@
+
+ struct RenamingFactory : public tooling::FrontendActionFactory {
+ RenamingFactory(RenamingFrontendAction *Action) : Action(Action) {
----------------
You don't actually need this.
Rename CreateASTConsumer to createASTConsumer, and you'll be able to just say:
newFrontendActionFactory(&Action)
below.
================
Comment at: clang-rename/USRFinder.cpp:41
@@ +40,3 @@
+ const SourceLocation Point,
+ const clang::NamedDecl **Result)
+ : SourceMgr(SourceMgr), LangOpts(LangOpts), Point(Point), Result(Result) {
----------------
I'd prefer giving this class a getResults() method over handing in a NamedDecl**.
================
Comment at: clang-rename/USRFinder.cpp:101-125
@@ +100,27 @@
+ if (const auto *Decl = Expr->getDirectCallee()) {
+ if (!isa<CXXOperatorCallExpr>(Expr) && (Decl->isOverloadedOperator() ||
+ isa<CXXConversionDecl>(Decl))) {
+ // If we got here, then we found an explicit operator call or
+ // conversion. We fix the range of such calls from the leading 'o' to
+ // the end of the actual operator.
+ const auto *Callee = Expr->getCallee();
+ const auto *MemExpr = dyn_cast<MemberExpr>(Callee);
+ // If this is a member expression, the start location should be after
+ // the '.' or '->'.
+ auto LocStart =
+ ((MemExpr) ? MemExpr->getMemberLoc() : Expr->getLocStart());
+ // getLocEnd returns the start of the last token in the callee
+ // expression. Thus, for 'operator +=', it will return the location of
+ // the '+'.
+ auto OpLoc = Callee->getLocEnd();
+ if (OpLoc.isValid() && OpLoc.isFileID()) {
+ SmallVector<char, 32> Buffer;
+ auto OpName = Lexer::getSpelling(OpLoc, Buffer, SourceMgr, LangOpts);
+ return setResult(Decl, LocStart,
+ OpLoc.getLocWithOffset(OpName.size() - 1));
+ }
+ // If we couldn't get the location of the end of the operator, we just
+ // check in the range of the keyword "operator".
+ return setResult(Decl, LocStart, strlen("operator"));
+ }
+ }
----------------
It's amazing how most of these functions seem to be handling overloaded operators :(
================
Comment at: clang-rename/USRFinder.cpp:209-212
@@ +208,6 @@
+ SourceLocation End) {
+ if (!Start.isValid() || !Start.isFileID() || !End.isValid() ||
+ !End.isFileID() || !isPointWithin(Start, End)) {
+ return true;
+ }
+ *Result = Decl;
----------------
Should we check that Start and End are in the same file?
================
Comment at: clang-rename/USRFinder.h:34
@@ +33,3 @@
+// Converts a Decl into a USR.
+std::string getUSRForDecl(const Decl *Decl);
+}
----------------
Nit: add newline below.
================
Comment at: clang-rename/USRLocFinder.cpp:40-48
@@ +39,11 @@
+
+ bool VisitNamedDecl(const NamedDecl *Decl) {
+ 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());
+ }
+ return true;
----------------
I think we either want to test this (which means it will not need a "This is correct as of" comment), or we don't want to rely on it.
================
Comment at: test/clang-rename/VarTest.cpp:3
@@ +2,3 @@
+namespace A {
+int /*0*/foo;
+}
----------------
I'm honestly not sure I like this better than:
// CHECK: int bar;
http://reviews.llvm.org/D4739
More information about the cfe-commits
mailing list