[PATCH] Clang Rename Tool
Matthew Plant
mplant at google.com
Tue Aug 5 09:52:57 PDT 2014
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> 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
> <javascript:_e(%7B%7D,'cvml','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
>> <javascript:_e(%7B%7D,'cvml','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/3dd3e039/attachment.html>
More information about the cfe-commits
mailing list