[PATCH] Clang Rename Tool
Matthew Plant
mplant at google.com
Mon Aug 4 14:41:20 PDT 2014
comments
================
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";
+ }
+
----------------
Manuel Klimek wrote:
> 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.
I agree, but I cannot refactor this to such a degree in such a short amount of time.
================
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);
+
----------------
Manuel Klimek wrote:
> What's the reason not to sink this into getLocationsOfUSR?
Because changing the location is only useful for renaming. If someone wants to use getLocationsOfUSR for anything else, they probably want a different location.
================
Comment at: clang-rename/ClangRename.cpp:215-216
@@ +214,4 @@
+ << FullLoc.getSpellingColumnNumber() << "\n";
+ Replaces.insert(tooling::Replacement(SourceMgr, Loc, PrevNameLen,
+ NewName));
+ }
----------------
Manuel Klimek wrote:
> 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...
That would best be left when more of the code is refactored sensibly and renamed locations are easy to extract. At least by printing to stderr the data can be sensibly removed from the output.
================
Comment at: clang-rename/ClangRename.cpp:262
@@ +261,3 @@
+
+ class RenamingFrontendAction : public ASTFrontendAction {
+ public:
----------------
Manuel Klimek wrote:
> 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.
I have done bits of the first parts, but future goals will have to wait until I move back to school
================
Comment at: clang-rename/ClangRename.cpp:283
@@ +282,3 @@
+
+ struct RenamingFactory : public tooling::FrontendActionFactory {
+ RenamingFactory(RenamingFrontendAction *Action) : Action(Action) {
----------------
Manuel Klimek wrote:
> You don't actually need this.
> Rename CreateASTConsumer to createASTConsumer, and you'll be able to just say:
> newFrontendActionFactory(&Action)
> below.
that is the weirdest interface in the world.
================
Comment at: clang-rename/USRFinder.cpp:41
@@ +40,3 @@
+ const SourceLocation Point,
+ const clang::NamedDecl **Result)
+ : SourceMgr(SourceMgr), LangOpts(LangOpts), Point(Point), Result(Result) {
----------------
Manuel Klimek wrote:
> I'd prefer giving this class a getResults() method over handing in a NamedDecl**.
Yeah I changed this. Really I think that this could be done a lot better with a few static methods, but I really don't have enough time for that within the next week. It'll have to wait for later in the year.
================
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"));
+ }
+ }
----------------
Manuel Klimek wrote:
> It's amazing how most of these functions seem to be handling overloaded operators :(
It was decided (not by me) that out of the two things left to possible add before up-streaming - macros and overloaded operators - the latter was to be completed.
Also, that's not entirely the case - most of the code doesn't, but the code that handles common cases (like operators and variables) are places where operator overloading needs to be accounted for.
================
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;
----------------
Manuel Klimek wrote:
> Should we check that Start and End are in the same file?
I guess...
================
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;
----------------
Manuel Klimek wrote:
> 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.
We don't rely on it. I'll remove the comment
================
Comment at: test/clang-rename/VarTest.cpp:3
@@ +2,3 @@
+namespace A {
+int /*0*/foo;
+}
----------------
Manuel Klimek wrote:
> I'm honestly not sure I like this better than:
> // CHECK: int bar;
This allows us to automatically find the location of the symbol. That way, it checks USRFinder's accuracy as well.
================
Comment at: test/clang-rename/VarTest.cpp:3
@@ +2,3 @@
+namespace A {
+int /*0*/foo;
+}
----------------
Matthew Plant wrote:
> Manuel Klimek wrote:
> > I'm honestly not sure I like this better than:
> > // CHECK: int bar;
> This allows us to automatically find the location of the symbol. That way, it checks USRFinder's accuracy as well.
this lets the tool automatically find locations. I assure you, it is much easier.
http://reviews.llvm.org/D4739
More information about the cfe-commits
mailing list