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@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 <[email protected]> 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 <[email protected] > <javascript:_e(%7B%7D,'cvml','[email protected]');>> 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 >> [email protected] >> <javascript:_e(%7B%7D,'cvml','[email protected]');> >> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >> > >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
