Page MenuHomePhabricator

[ASTMatchers] Allow hasName() to look through inline namespaces
ClosedPublic

Authored by sbenza on Dec 14 2015, 12:43 PM.

Details

Summary

Allow hasName() to look through inline namespaces.
This will fix the interaction between some clang-tidy checks and libc++.

libc++ defines names in an inline namespace named std::<version_#>.
When we try to match a name using hasName("std::xxx") it fails to match and the clang-tidy check does not work.

Should fix http://llvm.org/PR25804, http://llvm.org/PR25812, http://llvm.org/PR26133.

Diff Detail

Event Timeline

sbenza updated this revision to Diff 42755.Dec 14 2015, 12:43 PM
sbenza retitled this revision from to [ASTMatchers] Allow hasName() to look through inline namespaces.
sbenza updated this object.
sbenza added a reviewer: klimek.
sbenza added a subscriber: cfe-commits.
aaron.ballman added inline comments.
lib/ASTMatchers/ASTMatchersInternal.cpp
320–331

Cute, but this won't work with MSVC 2013. You'll get: error C3312: no callable 'begin' function found for type 'initializer-list'

335

Should elide braces and format using clang-format.

MSVC 2013 Update 5 accepts for (bool SkipUnwritten : {false, true}).
Possibly changed in one of the Updates?

sbenza updated this revision to Diff 42767.Dec 14 2015, 1:50 PM

Minor fixes

I am thinking on doing this some other way.
Copying the PrintingPolicy object is very expensive, as it contains quite a few strings and vectors of strings.
In the past I changed this matcher to make no allocations in the common path (ie using SmallString<>, etc) to reduce its cost.
Adding that copy will make it expensive again.

Also, this change is not completely correct. It either uses all namespaces or none of the inline.
It won't partially match.

Eg:

namespace A {
inline namespace B {
inline namespace C {
void F();
}
}
}

We should be able to match F as A::C::F and that doesn't work right now.

lib/ASTMatchers/ASTMatchersInternal.cpp
320–331

Thanks. I assumed it worked because we already allow range-for in LLVM.
Extracted the array into a variable.

335

Which braces do you want to remove?
I need these to make the else bind with the outer if().

Applied clang-format.

aaron.ballman added inline comments.Dec 14 2015, 1:59 PM
lib/ASTMatchers/ASTMatchersInternal.cpp
320–331

Extracted the array into a variable.

Good solution to be on the safe side. Range-for isn't the issue with MSVC 2013, it's initializer lists (which really didn't get enough love until 2015).

335

Which braces do you want to remove?
I need these to make the else bind with the outer if().

Good point; I think they're fine as-is then.

yaron.keren added inline comments.Dec 14 2015, 2:02 PM
lib/ASTMatchers/ASTMatchersInternal.cpp
322

StringRef not needed, simply

llvm::SmallString<128> NodeName = "::";
323–331

You may be able to do without the if

PrintingPolicy Policy = Node.getASTContext().getPrintingPolicy();
Policy.SuppressUnwrittenScope = SkipUnwritten;
Node.printQualifiedName(OS, Policy);
sbenza updated this revision to Diff 42884.Dec 15 2015, 11:44 AM

Add a faster version of the qualified name matcher.
It falls back to the previous version if it can't handle the name.

lib/ASTMatchers/ASTMatchersInternal.cpp
322

I tried it before. It doesn't work.

/usr/local/google/llvm_git/llvm/tools/clang/lib/ASTMatchers/ASTMatchersInternal.cpp:321:28: error: no viable conversion from 'const char [3]' to

  'llvm::SmallString<128>'
llvm::SmallString<128> NodeName = "::";

It'll be great to finalize fix before 3.8 branching (planned at January 13).

alexfh added a subscriber: alexfh.Jan 15 2016, 6:12 AM

Manuel, can you take a look at this?

Adding Benjamin for an additional pair of eyes.
Generally looks good.

lib/ASTMatchers/ASTMatchersInternal.cpp
335

Perhaps comment that Pattern will be changed to the prefix, so it's clear afterwards.
(I wonder whether returning a tuple might be slightly easier to understand here)

alexfh updated this object.Jan 15 2016, 7:38 AM
alexfh edited edge metadata.
sbenza updated this revision to Diff 45438.Jan 20 2016, 1:36 PM

Rename functions.

lib/ASTMatchers/ASTMatchersInternal.cpp
335

Renamed from Match to Consume. Is that better?

Prazek added a subscriber: Prazek.Feb 4 2016, 12:37 PM

What is the status of it?

bkramer accepted this revision.Feb 5 2016, 7:34 AM
bkramer edited edge metadata.

lg

This revision is now accepted and ready to land.Feb 5 2016, 7:34 AM
This revision was automatically updated to reflect the committed changes.