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