ioeric accepted this revision. ioeric added a comment. This revision is now accepted and ready to land.
Lg ================ Comment at: lib/Tooling/Refactoring/Rename/USRLocFinder.cpp:212 + // Ignore the case where there is no prefix qualifer for the enum constant + // expression like `a = Green`. + if (!Expr->hasQualifier()) ---------------- hokein wrote: > ioeric wrote: > > hokein wrote: > > > ioeric wrote: > > > > Why do we ignore this case? What if `Color` is moved to a different > > > > namespace? We would also need to qualify `Green` with a new namespace. > > > Thanks for pointing it out (we missed such test case before). > > > > > > Yeah, this should be a FIXME, and added the case to the unittest. > > It would be nice if this could also be fixed in this patch; I think it > > might affect the current code structure. This doesn't seem to be a very > > hard case to fix after all? > To fix this case , we have to insert the prefix qualifier at the BeginLoc, > which is not supported by the current code. > I prefer to do it in a follow-up patch. Sure. Up to you. ================ Comment at: lib/Tooling/Refactoring/Rename/USRLocFinder.cpp:228 + // enum constant `Green` should be excluded). + // ns1::ns2::Green; + // ^ ^ ^ ---------------- hokein wrote: > ioeric wrote: > > hokein wrote: > > > ioeric wrote: > > > > I'm wondering why the old `EndLoc` points to `G` instead of `n` in > > > > `Green`. > > > This is the behavior of `Expr->getLocEnd()` or > > > `Expr->getSourceRange().getEnd()`, which returns the start location of > > > the last token. > > I'm a bit nervous about the implementation - it feels a bit too hacky... > > > > Would it be possible to get the range for the qualifier `ns1::ns2::` from > > the `DeclRefExpr`? > Switched to use `getQualifierLoc`, we still need to exclude the last `::` by > moving 1 character backward. It might also make sense to double check that the old end loc is pointing at "::". https://reviews.llvm.org/D38989 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
