This is an archive of the discontinued LLVM Phabricator instance.

[clang-rename] Rename enum.
ClosedPublic

Authored by hokein on Oct 17 2017, 12:57 AM.

Details

Summary
  • Add unit tests for renaming enum.
  • Support unscoped enum constants in expressions.

Event Timeline

hokein created this revision.Oct 17 2017, 12:57 AM
ioeric added inline comments.Oct 17 2017, 1:37 AM
lib/Tooling/Refactoring/Rename/USRLocFinder.cpp
212

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.

218

nit: no braces

228

I'm wondering why the old EndLoc points to G instead of n in Green.

hokein updated this revision to Diff 119274.Oct 17 2017, 2:43 AM
hokein marked 2 inline comments as done.

address review comments, and add FIXME

hokein added inline comments.Oct 17 2017, 2:49 AM
lib/Tooling/Refactoring/Rename/USRLocFinder.cpp
212

Thanks for pointing it out (we missed such test case before).

Yeah, this should be a FIXME, and added the case to the unittest.

228

This is the behavior of Expr->getLocEnd() or Expr->getSourceRange().getEnd(), which returns the start location of the last token.

ioeric added inline comments.Oct 17 2017, 3:24 AM
lib/Tooling/Refactoring/Rename/USRLocFinder.cpp
212

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?

228

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?

hokein updated this revision to Diff 119309.Oct 17 2017, 6:24 AM

Use getQualifierLoc.

lib/Tooling/Refactoring/Rename/USRLocFinder.cpp
212

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.

228

Switched to use getQualifierLoc, we still need to exclude the last :: by moving 1 character backward.

ioeric accepted this revision.Oct 17 2017, 6:29 AM

Lg

lib/Tooling/Refactoring/Rename/USRLocFinder.cpp
212

Sure. Up to you.

228

It might also make sense to double check that the old end loc is pointing at "::".

This revision is now accepted and ready to land.Oct 17 2017, 6:29 AM
This revision was automatically updated to reflect the committed changes.