Page MenuHomePhabricator

[clang-rename] introduce better symbol finding and add few more tests
ClosedPublic

Authored by omtcyfz on Jul 18 2016, 8:22 AM.

Details

Summary

This patch introduces:

  • TypeLoc visiting, which helps a lot in renaming types
  • NestedNameSpecifierLoc visiting (through getting them via ASTMatcher at the moment, though, because RecursiveASTVisitor<T>::VisitNestedNameSpecifierLoc isn't implemented), which helps to treat nested names correctly
  • few refactoring changes

Diff Detail

Repository
rL LLVM

Event Timeline

omtcyfz updated this revision to Diff 64325.Jul 18 2016, 8:22 AM
omtcyfz retitled this revision from to [clang-rename] introduce better symbol finding and add few more tests.
omtcyfz updated this object.
omtcyfz added reviewers: klimek, bkramer, alexfh.
omtcyfz added a subscriber: cfe-commits.
omtcyfz updated this revision to Diff 64327.Jul 18 2016, 8:27 AM

add XFAIL to currently unsupported test

omtcyfz updated this revision to Diff 64374.Jul 18 2016, 1:24 PM

Removed two FIXMEs. Seems like types are now handled correctly. +1 test passing.

omtcyfz updated this revision to Diff 64454.Jul 19 2016, 12:54 AM

applied few stylistic fixes

Can anyone please take a look at this?

My current work is based on this patch and I'd be happy to know I'm doing things right :)

omtcyfz updated this revision to Diff 64668.Jul 20 2016, 5:22 AM

add support for renaming inside NestedNameSpecifier's

omtcyfz updated this object.Jul 20 2016, 5:24 AM
omtcyfz updated this revision to Diff 64838.Jul 21 2016, 2:04 AM

few fixes

omtcyfz updated this revision to Diff 64839.Jul 21 2016, 2:07 AM

fix previously unsupported test

omtcyfz updated this revision to Diff 64841.Jul 21 2016, 2:16 AM

add one more XFAIL test

omtcyfz updated this revision to Diff 64844.Jul 21 2016, 2:19 AM
alexfh requested changes to this revision.Jul 21 2016, 6:35 PM
alexfh edited edge metadata.
alexfh added inline comments.
clang-rename/USRFinder.cpp
81 ↗(On Diff #64454)

Declarations of multiple variables can be confusing. Please split this declaration.

94 ↗(On Diff #64844)

nit: No need for braces around single-line if bodies.

clang-rename/USRFinder.h
49 ↗(On Diff #64844)

We should implement it then ;)

test/clang-rename/ComplicatedClassType.cpp
1 ↗(On Diff #64844)

I guess, this test will need // REQUIRES: shell. You can try to commit without it, but watch windows buildbots closely.

This revision now requires changes to proceed.Jul 21 2016, 6:35 PM
omtcyfz updated this revision to Diff 65033.Jul 22 2016, 1:33 AM
omtcyfz edited edge metadata.
omtcyfz marked an inline comment as done.

Address comments.

omtcyfz added inline comments.Jul 22 2016, 1:35 AM
clang-rename/USRFinder.cpp
94 ↗(On Diff #64844)

Well, I'm really happy with braces everywhere. The codestyle doesn't prohibit it and I think it's more readable than no braces around single statements in logical blocks.

clang-rename/USRFinder.h
49 ↗(On Diff #64844)

Yes, it's in my TODO list. But I suppose it's better to fix clang-rename first and get back to it a bit later.

As we discussed, there will probably be no need in RecursiveASTVisitors at all in the end.

Anyway, the patch is already in the state when nobody wants to review it :D I'd be happy just to land it and to continue with all the other bugs present in the tool.

test/clang-rename/ComplicatedClassType.cpp
1 ↗(On Diff #64844)

This test? There's RUN: cat %s > %t.cpp in every single one of them, I'm not sure what's the issue.

omtcyfz updated this revision to Diff 65053.Jul 22 2016, 4:56 AM
omtcyfz edited edge metadata.
  • reduce rate of hardcoding
  • simplify symbol location finding
  • introduce tests with templates, some of them are PASSing
omtcyfz updated this revision to Diff 65054.Jul 22 2016, 4:59 AM

split declarations

omtcyfz updated this revision to Diff 65056.Jul 22 2016, 5:01 AM

oops... actually split declarations

omtcyfz updated this revision to Diff 65060.Jul 22 2016, 5:41 AM

remove redundant #include and using directives

alexfh accepted this revision.Jul 22 2016, 6:03 AM
alexfh edited edge metadata.

LG. Thanks!

clang-rename/USRFinder.h
49 ↗(On Diff #65056)

SG. Please change the comment to // FIXME: Implement RecursiveASTVisitor<T>::VisitNestedNameSpecifier instead..

test/clang-rename/ComplicatedClassType.cpp
1–2 ↗(On Diff #65056)

Actually, cat a > b is a way to write cp a b in lit tests without requiring a shell. I looked this up and was going to delete the comment, but I've hit "Hide" instead. Sorry for the noise.

This revision is now accepted and ready to land.Jul 22 2016, 6:03 AM
omtcyfz updated this revision to Diff 65065.Jul 22 2016, 6:27 AM
omtcyfz edited edge metadata.

@alexfh, awww, sorry, I've uploaded wrong diff last time for whatever reason...

Can you review the changes please?

Now it contains many new tests and I managed to cut off unnecessary blocks of code.

omtcyfz updated this revision to Diff 65066.Jul 22 2016, 6:32 AM

@alexfh, or wait... It's fine, I just forgot to add few tests, sorry.

test/clang-rename/ComplicatedClassType.cpp
1–6 ↗(On Diff #65060)

No problem :)

This revision was automatically updated to reflect the committed changes.