This is an archive of the discontinued LLVM Phabricator instance.

Adds hasUnqualifiedDesugaredType to allow matching through type sugar.
ClosedPublic

Authored by klimek on Nov 29 2016, 5:23 AM.

Event Timeline

klimek updated this revision to Diff 79547.Nov 29 2016, 5:23 AM
klimek retitled this revision from to Adds hasUnqualifiedDesugaredType to allow matching through type sugar..
klimek updated this object.
klimek added reviewers: bkramer, lukasza.
klimek added a subscriber: cfe-commits.
lukasza edited edge metadata.Nov 29 2016, 2:14 PM

I've tried replicating the deep-matching by saying qualType(hasType(hasUnqualifiedDesugaredType(hasDeclaration(... but this doesn't work because hasDeclaration only returns a matcher for a specific type from a subset of subclasses of Type - this is incompatible with expectations of the proposed hasUnqualifiedDesugaredType which takes a Matcher<Type>.

FWIW, the following matcher worked for me:

AST_MATCHER_P(Type, hasUnqualifiedDesugaredType, internal::Matcher<QualType>,
              InnerMatcher) {
  const Type* type = Node.getUnqualifiedDesugaredType();
  QualType qualType(type, 0);
  return InnerMatcher.matches(qualType, Finder, Builder);
}

TEST(HasDeclaration, DeepTagType) {
  std::string input =
      "class Foo {};\n"
      "using Bar = Foo;\n"
      "using Baz = Bar;\n"
      "void Function(Baz param) {}\n";

  // Matcher for declaration of the Foo class.
  auto param_type_decl_matcher = cxxRecordDecl(hasName("Foo"));

  auto m1 = qualType(hasDeclaration(decl(param_type_decl_matcher)));
  // hasDeclaration / qualType-flavour.
  EXPECT_TRUE(matches(input, parmVarDecl(
      hasName("param"),
      hasType(hasUnqualifiedDesugaredType(m1)))));
}

I think the above will work for my tool - thank you for providing the matcher example (for some reason I incorrectly thought that desugaring would only be done one step a time - this made me think that matching at an arbitrary depth will require tricky things to build a recursive matcher out of a single-step matcher).

BTW: Please note that I am fine with just copy&pasting the matcher above directly into my tool - I don't necessarily need this matcher to be part of the generic library of matchers. I don't have high confidence that the matcher here is 1) applicable broadly (maybe) and 2) what to do about the Type vs QualType issue (yes, it says "unqualified" right in the name, but then it means that it cannot be used with hasDeclaration...).

I think the above will work for my tool - thank you for providing the matcher example (for some reason I incorrectly thought that desugaring would only be done one step a time - this made me think that matching at an arbitrary depth will require tricky things to build a recursive matcher out of a single-step matcher).

clang-tidy's modernize-use-auto check has a matcher that desugars one step at a time:
https://reviews.llvm.org/diffusion/L/browse/clang-tools-extra/trunk/clang-tidy/modernize/UseAutoCheck.cpp;288204$69

lukasza added inline comments.Nov 29 2016, 2:25 PM
unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
253

deep testing suggestion

If we do end up landing the implementation of hasUnqualifiedDesugaredType from this CL, then I think it might be good to tweak the test so that it verifies that the matching here is "deep" (i.e. that the matcher doesn't just strip a single level of sugar).

matches("struct A {}; using B = A; using C = B; C c;", ...

deep-vs-shallow-vs-everythingInBetween

At one point, when discussing the old implementation of hasDeclaration (before https://reviews.llvm.org/D27104) we were wondering whether it should match for all the cases below:

std::string input = "struct A {}; using B = A; using C = B; using D = C; D d;";
EXPECT_TRUE(matches(input,
    varDecl(...something-hasDeclaration-something...(typeAliasDecl(hasName("B"))))));
EXPECT_TRUE(matches(input,
    varDecl(...something-hasDeclaration-something...(typeAliasDecl(hasName("C"))))));
EXPECT_TRUE(matches(input,
    varDecl(...something-hasDeclaration-something...(typeAliasDecl(hasName("D"))))));

This is not something I want or care about myself, but I wanted to check if the above is something to think about. For example - maybe exposing a singleStepDesugarsTo(...) matcher is better (because it can be used to build the hasUnqualifiedDesugaredType).

As I said - this is not very important to me (I only care about full desugaring all the way to a tag type), but it might be something to ponder before committing to the current implementation of hasUnqualifiedDesugaredType.

hasUnqualifiedDesugaredType(hasDeclaration(

How about using hasUnqualifiedDesugaredType(recordType(hasDeclaration instead?

klimek updated this revision to Diff 79891.Dec 1 2016, 3:44 AM
klimek edited edge metadata.

Add multi-level using test.

klimek added a comment.Dec 1 2016, 4:00 AM

I added a test for multi-stage desugaring. Generally, we want to model Clang's AST where it makes sense. The AST has a getUnqualifiedDesugaredType, thus the matcher as it is is expected. We can add single step desugaring or hasAnyDeclaration or similar things when they are actually needed.

This question from my previous comment still stands:

How about using hasUnqualifiedDesugaredType(recordType(hasDeclaration instead?

bkramer accepted this revision.Dec 1 2016, 5:20 AM
bkramer edited edge metadata.

This looks good to me. We can add finer-grained desugaring later if it's really needed.

This revision is now accepted and ready to land.Dec 1 2016, 5:20 AM
This revision was automatically updated to reflect the committed changes.
alexfh added a subscriber: alexfh.Aug 3 2017, 7:12 AM
alexfh added inline comments.
cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h
2749 ↗(On Diff #79912)

Typo: hasUniqualifeidDesugaredType (actually, two typos).