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

[Haojian Wu via Phabricator via cfe-commits]

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