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

[Eric Liu via Phabricator via cfe-commits]

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