Page MenuHomePhabricator

[Tooling] Add a utility function to replace one nested name with another.
ClosedPublic

Authored by bkramer on Oct 21 2015, 2:46 AM.

Details

Summary

One problem in clang-tidy and other clang tools face is that there is no
way to lookup an arbitrary name in the AST, that's buried deep inside Sema
and might not even be what the user wants as the new name may be freshly
inserted and not available in the AST.

A common use case for lookups is replacing one nested name with another
while minimizing namespace qualifications, so replacing 'ns::foo' with
'ns::bar' will use just 'bar' if we happen to be inside the namespace 'ns'.
This adds a little helper utility for exactly that use case.

Diff Detail

Repository
rL LLVM

Event Timeline

bkramer updated this revision to Diff 37976.Oct 21 2015, 2:46 AM
bkramer retitled this revision from to [Tooling] Add a utility function to replace one nested name with another..
bkramer updated this object.
bkramer added a reviewer: klimek.
bkramer added a subscriber: cfe-commits.
klimek added inline comments.Oct 21 2015, 5:04 AM
include/clang/Tooling/Core/Lookup.h
37–38 ↗(On Diff #37976)

Don't most tests violate the "should be fully qualified" requirement here?

lib/Tooling/Core/Lookup.cpp
60 ↗(On Diff #37976)

s/of/off/

63–66 ↗(On Diff #37976)

If NewName is fully qualified, why would we ever want to allow that to match from a non-global namespace?

namespace a { namespace b { namespace c {
void foo();
}}}

-> if NewName is "::c::bar" (for whatever reason), this should probably not silently go to bar inside 'c', or to c::bar inside 'b'.

72 ↗(On Diff #37976)

Isn't that more like isFullyQualified then?

unittests/Tooling/LookupTest.cpp
19–20 ↗(On Diff #37976)

Is this ever not true?

bkramer updated this revision to Diff 38117.Oct 22 2015, 5:36 AM
  • Removed "fully qualified" in favor of just "qualified" to clarify that the name should be qualified but the leading "::" is not necessary.
  • Renamed isNameSpecifierGlobal
  • Removed always true conditional from test
bkramer updated this revision to Diff 38122.Oct 22 2015, 6:28 AM

Add more comments and polish test cases.

klimek added inline comments.Oct 22 2015, 7:50 AM
include/clang/Tooling/Core/Lookup.h
37–38 ↗(On Diff #38122)

After some pondering, I think we should require a fully qualified name, as then it's really unambiguous from the call site what is happening.

unittests/Tooling/LookupTest.cpp
69–73 ↗(On Diff #38122)

Do we already have a test that tests c::bar with replaceCallExpr(Expr, "a::c::bar"), or do you think that obviously works from this test?

bkramer updated this revision to Diff 38130.Oct 22 2015, 7:57 AM

Enforce :: at the beginning of the new name.

klimek accepted this revision.Oct 22 2015, 8:02 AM
klimek edited edge metadata.

lg

This revision is now accepted and ready to land.Oct 22 2015, 8:02 AM
This revision was automatically updated to reflect the committed changes.