[PATCH] D38989: [clang-rename] Rename enum.

[Eric Liu via Phabricator via cfe-commits]

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