[PATCH] Clang Rename Tool
Matthew Plant
mplant at google.com
Tue Aug 5 10:20:33 PDT 2014
So if that last email was at all ambitious; yes I am planning to write
follow up patches. I actually really want to write a USR AST marcher -
imagine the possibilities for editors!
On Tuesday, August 5, 2014, Matthew Plant <mplant at google.com> wrote:
> I have no intention of ignoring reviews. I should mention that I am only
> at google for another week. I am happy to continue the review from my
> maplant2 at illinois email afterwards, but there are some things that I
> simply cannot change within a week. Remaking the visitors AST matches for
> one is just something I can't do in a week, but could potentially do
> afterwards. I'm sorry Manuel it you got the wrong impression, I have no
> intention to stop the review.
>
> But if you were to stop the review, I imagine it would be the death of
> clang rename.
>
> On Tuesday, August 5, 2014, Chandler Carruth <chandlerc at google.com
> <javascript:_e(%7B%7D,'cvml','chandlerc at google.com');>> wrote:
>
>> Sorry if I've gotten the wrong impression here, but reading this review
>> it seemed like the review comments weren't really being taken seriously...
>>
>> The Clang tools stuff is an open source project that you're contributing
>> to. I realize the time you have to contribute things may be limited, but
>> the project can't really adjust its code review, design standards, or code
>> quality to work around that. I would encourage you to actually respond to
>> the comments Manuel has provided to the best of your ability in whatever
>> time you're interested in putting into this. Ultimately, before this goes
>> in, *someone* is going to have to do the work to refactor and design this
>> code the right way. Maybe someone else will pick up the patch, but either
>> way, I think the code review stands and it is on you or whomever else to
>> address it. You can't skip that part of the review because you don't have
>> time.
>>
>> (Now, if I've misunderstood, and you're actually just planning to revisit
>> these issues with a follow-up patch, that's a different matter and sorry
>> for any confusion.)
>>
>>
>> On Mon, Aug 4, 2014 at 2:41 PM, Matthew Plant <mplant at google.com> wrote:
>>
>>> 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
>>>
>>>
>>>
>>> _______________________________________________
>>> cfe-commits mailing list
>>> cfe-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>>
>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140805/93b0f1d1/attachment.html>
More information about the cfe-commits
mailing list