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