<div dir="ltr"><div>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...</div><div><br></div><div>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.</div>
<div><br></div><div>(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.)</div></div><div class="gmail_extra">
<br><br><div class="gmail_quote">On Mon, Aug 4, 2014 at 2:41 PM, Matthew Plant <span dir="ltr"><<a href="mailto:mplant@google.com" target="_blank">mplant@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
comments<br>
<div><div class="h5"><br>
================<br>
Comment at: clang-rename/ClangRename.cpp:159-188<br>
@@ +158,32 @@<br>
+<br>
+ if (PrevName.empty()) {<br>
+ // Retrieve the previous name and USRs.<br>
+ // The source file we are searching will always be the main source file.<br>
+ const auto Point = SourceMgr.getLocForStartOfFile(<br>
+ SourceMgr.getMainFileID()).getLocWithOffset(SymbolOffset);<br>
+ const NamedDecl *TargetDecl = getNamedDeclAt(Context, Point);<br>
+ if (TargetDecl == nullptr) {<br>
+ errs() << "clang-rename: no symbol found at given location\n";<br>
+ *Failed = true;<br>
+ return;<br>
+ }<br>
+<br>
+ // If the decl is in any way related to a class, we want to make sure we<br>
+ // search for the constructor and destructor as well as everything else.<br>
+ if (const auto *CtorDecl = dyn_cast<CXXConstructorDecl>(TargetDecl))<br>
+ TargetDecl = CtorDecl->getParent();<br>
+ else if (const auto *DtorDecl = dyn_cast<CXXDestructorDecl>(TargetDecl))<br>
+ TargetDecl = DtorDecl->getParent();<br>
+<br>
+ if (const auto *Record = dyn_cast<CXXRecordDecl>(TargetDecl)) {<br>
+ auto ExtraUSRs = getAllConstructorUSRs(Record);<br>
+ USRs.insert(USRs.end(), ExtraUSRs.begin(), ExtraUSRs.end());<br>
+ }<br>
+<br>
+ // Get the primary USR and previous symbol's name.<br>
+ USRs.push_back(getUSRForDecl(TargetDecl));<br>
+ PrevName = TargetDecl->getNameAsString();<br>
+ if (PrintName)<br>
+ errs() << "clang-rename: found symbol: " << PrevName << "\n";<br>
+ }<br>
+<br>
----------------<br>
</div></div><div class="">Manuel Klimek wrote:<br>
> 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.<br>
</div>I agree, but I cannot refactor this to such a degree in such a short amount of time.<br>
<div class=""><br>
================<br>
Comment at: clang-rename/ClangRename.cpp:201-206<br>
@@ +200,8 @@<br>
+<br>
+ // If we're renaming an operator, we need to fix some of our source<br>
+ // locations.<br>
+ auto OpShortForm = getOpCallShortForm(PrevName);<br>
+ if (!OpShortForm.empty())<br>
+ RenamingCandidates =<br>
+ fixOperatorLocs(OpShortForm, RenamingCandidates, SourceMgr, LangOpts);<br>
+<br>
----------------<br>
</div><div class="">Manuel Klimek wrote:<br>
> What's the reason not to sink this into getLocationsOfUSR?<br>
</div>Because changing the location is only useful for renaming. If someone wants to use getLocationsOfUSR for anything else, they probably want a different location.<br>
<div class=""><br>
================<br>
Comment at: clang-rename/ClangRename.cpp:215-216<br>
@@ +214,4 @@<br>
+ << FullLoc.getSpellingColumnNumber() << "\n";<br>
+ Replaces.insert(tooling::Replacement(SourceMgr, Loc, PrevNameLen,<br>
+ NewName));<br>
+ }<br>
----------------<br>
</div><div class="">Manuel Klimek wrote:<br>
> 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...<br>
</div>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.<br>
<div class=""><br>
================<br>
Comment at: clang-rename/ClangRename.cpp:262<br>
@@ +261,3 @@<br>
+<br>
+ class RenamingFrontendAction : public ASTFrontendAction {<br>
+ public:<br>
----------------<br>
</div><div class="">Manuel Klimek wrote:<br>
> I'd prefer this class (and everything it uses) to go into its own .h/.cpp file.<br>
> Perhaps call it RenamingAction.h/.cpp.<br>
><br>
> I'd still want to modularize it further:<br>
> - One action to find the USRs (perhaps just export it from USRFinder.h)<br>
> - (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)<br>
> - most of the code in RenamingASTConsumer seems non-renaming related; see comments there.<br>
</div>I have done bits of the first parts, but future goals will have to wait until I move back to school<br>
<div class=""><br>
================<br>
Comment at: clang-rename/ClangRename.cpp:283<br>
@@ +282,3 @@<br>
+<br>
+ struct RenamingFactory : public tooling::FrontendActionFactory {<br>
+ RenamingFactory(RenamingFrontendAction *Action) : Action(Action) {<br>
----------------<br>
</div><div class="">Manuel Klimek wrote:<br>
> You don't actually need this.<br>
> Rename CreateASTConsumer to createASTConsumer, and you'll be able to just say:<br>
> newFrontendActionFactory(&Action)<br>
> below.<br>
</div>that is the weirdest interface in the world.<br>
<div class=""><br>
<br>
================<br>
Comment at: clang-rename/USRFinder.cpp:41<br>
@@ +40,3 @@<br>
+ const SourceLocation Point,<br>
+ const clang::NamedDecl **Result)<br>
+ : SourceMgr(SourceMgr), LangOpts(LangOpts), Point(Point), Result(Result) {<br>
----------------<br>
</div><div class="">Manuel Klimek wrote:<br>
> I'd prefer giving this class a getResults() method over handing in a NamedDecl**.<br>
</div>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.<br>
<div><div class="h5"><br>
================<br>
Comment at: clang-rename/USRFinder.cpp:101-125<br>
@@ +100,27 @@<br>
+ if (const auto *Decl = Expr->getDirectCallee()) {<br>
+ if (!isa<CXXOperatorCallExpr>(Expr) && (Decl->isOverloadedOperator() ||<br>
+ isa<CXXConversionDecl>(Decl))) {<br>
+ // If we got here, then we found an explicit operator call or<br>
+ // conversion. We fix the range of such calls from the leading 'o' to<br>
+ // the end of the actual operator.<br>
+ const auto *Callee = Expr->getCallee();<br>
+ const auto *MemExpr = dyn_cast<MemberExpr>(Callee);<br>
+ // If this is a member expression, the start location should be after<br>
+ // the '.' or '->'.<br>
+ auto LocStart =<br>
+ ((MemExpr) ? MemExpr->getMemberLoc() : Expr->getLocStart());<br>
+ // getLocEnd returns the start of the last token in the callee<br>
+ // expression. Thus, for 'operator +=', it will return the location of<br>
+ // the '+'.<br>
+ auto OpLoc = Callee->getLocEnd();<br>
+ if (OpLoc.isValid() && OpLoc.isFileID()) {<br>
+ SmallVector<char, 32> Buffer;<br>
+ auto OpName = Lexer::getSpelling(OpLoc, Buffer, SourceMgr, LangOpts);<br>
+ return setResult(Decl, LocStart,<br>
+ OpLoc.getLocWithOffset(OpName.size() - 1));<br>
+ }<br>
+ // If we couldn't get the location of the end of the operator, we just<br>
+ // check in the range of the keyword "operator".<br>
+ return setResult(Decl, LocStart, strlen("operator"));<br>
+ }<br>
+ }<br>
----------------<br>
</div></div><div class="">Manuel Klimek wrote:<br>
> It's amazing how most of these functions seem to be handling overloaded operators :(<br>
</div>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.<br>
<br>
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.<br>
<div class=""><br>
================<br>
Comment at: clang-rename/USRFinder.cpp:209-212<br>
@@ +208,6 @@<br>
+ SourceLocation End) {<br>
+ if (!Start.isValid() || !Start.isFileID() || !End.isValid() ||<br>
+ !End.isFileID() || !isPointWithin(Start, End)) {<br>
+ return true;<br>
+ }<br>
+ *Result = Decl;<br>
----------------<br>
</div><div class="">Manuel Klimek wrote:<br>
> Should we check that Start and End are in the same file?<br>
</div>I guess...<br>
<div class=""><br>
================<br>
Comment at: clang-rename/USRLocFinder.cpp:40-48<br>
@@ +39,11 @@<br>
+<br>
+ bool VisitNamedDecl(const NamedDecl *Decl) {<br>
+ if (getUSRForDecl(Decl) == USR) {<br>
+ // Because we traverse the AST from top to bottom, it follows that the<br>
+ // current locations must be further down (or right) than the previous one<br>
+ // inspected. This has the effect of keeping LocationsFound sorted by<br>
+ // line first column second with no effort of our own.<br>
+ // This is correct as of 2014-06-27<br>
+ LocationsFound.push_back(Decl->getLocation());<br>
+ }<br>
+ return true;<br>
----------------<br>
</div><div class="">Manuel Klimek wrote:<br>
> 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.<br>
</div>We don't rely on it. I'll remove the comment<br>
<div class=""><br>
================<br>
Comment at: test/clang-rename/VarTest.cpp:3<br>
@@ +2,3 @@<br>
+namespace A {<br>
+int /*0*/foo;<br>
+}<br>
----------------<br>
</div><div class="">Manuel Klimek wrote:<br>
> I'm honestly not sure I like this better than:<br>
> // CHECK: int bar;<br>
</div>This allows us to automatically find the location of the symbol. That way, it checks USRFinder's accuracy as well.<br>
<div class=""><br>
================<br>
Comment at: test/clang-rename/VarTest.cpp:3<br>
@@ +2,3 @@<br>
+namespace A {<br>
+int /*0*/foo;<br>
+}<br>
----------------<br>
</div><div class="">Matthew Plant wrote:<br>
> Manuel Klimek wrote:<br>
</div><div class="">> > I'm honestly not sure I like this better than:<br>
> > // CHECK: int bar;<br>
</div>> This allows us to automatically find the location of the symbol. That way, it checks USRFinder's accuracy as well.<br>
this lets the tool automatically find locations. I assure you, it is much easier.<br>
<div class="HOEnZb"><div class="h5"><br>
<a href="http://reviews.llvm.org/D4739" target="_blank">http://reviews.llvm.org/D4739</a><br>
<br>
<br>
<br>
_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
</div></div></blockquote></div><br></div>