[PATCH] Clang Rename Tool
Chandler Carruth
chandlerc at google.com
Tue Aug 5 00:47:40 PDT 2014
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/c62b180b/attachment.html>
More information about the cfe-commits
mailing list