hokein added inline comments.
================ 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()) ---------------- 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. ================ Comment at: lib/Tooling/Refactoring/Rename/USRLocFinder.cpp:228 + // enum constant `Green` should be excluded). + // ns1::ns2::Green; + // ^ ^ ^ ---------------- 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. https://reviews.llvm.org/D38989 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
