Page MenuHomePhabricator

Add `TypeLoc`-related matchers.
ClosedPublic

Authored by jcking1034 on Oct 6 2021, 9:56 AM.

Details

Summary

Contributes several matchers that involve TypeLocs. These matchers are (in alphabetical order):

  • elaboratedTypeLoc
  • hasAnyTemplateArgumentLoc
  • hasNamedTypeLoc
  • hasPointeeLoc
  • hasReferentLoc
  • hasReturnTypeLoc
  • hasTemplateArgumentLoc
  • hasUnqualifiedLoc
  • pointerTypeLoc
  • qualifiedTypeLoc
  • referenceTypeLoc
  • templateSpecializationTypeLoc

Diff Detail

Event Timeline

jcking1034 requested review of this revision.Oct 6 2021, 9:56 AM
jcking1034 created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptOct 6 2021, 9:56 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Update dynamic matcher registry; update matcher page; remove unnecessary const qualifiers

Nice! Just some minor notes.

clang/docs/LibASTMatchersReference.html
636

Here and below. These changes look wrong. Did the script encounter an error?

clang/include/clang/ASTMatchers/ASTMatchers.h
6358

I think you need a const here (to the right) to be considered a qualified type loc.

6400

I think int not int*?

6470

why the use of std::function? Seems easier just to take Finder and Builder directly as arguments?

6526–6528

Maybe simplify this -- I think a less detailed matcher would still be a useful illustration.

Also, please list the new matchers in the patch description.

jcking1034 edited the summary of this revision. (Show Details)Oct 6 2021, 12:41 PM
jcking1034 updated this revision to Diff 377667.Oct 6 2021, 1:32 PM
jcking1034 marked 3 inline comments as done.
jcking1034 edited the summary of this revision. (Show Details)

Improve comments and examples; simplify hasTemplateArgumentLoc implementation.

jcking1034 marked an inline comment as not done.Oct 6 2021, 1:34 PM
jcking1034 added inline comments.
clang/include/clang/ASTMatchers/ASTMatchers.h
6358

I've decided to use a different example to be a bit clearer.

6400

I think this should be correct. If we were to look at int** x, pointerTypeLoc(hasPointeeLoc(pointerTypeLoc(hasPointeeLoc(loc(asString("int")))))) will match the whole TypeLoc and pointerTypeLoc(hasPointeeLoc(loc(asString("int")))) will match int*.

ymandel accepted this revision.Oct 6 2021, 1:42 PM
ymandel added inline comments.
clang/include/clang/ASTMatchers/ASTMatchers.h
6400

Sorry -- misread -- i was focused on the pointee part. you're absolutely right.

This revision is now accepted and ready to land.Oct 6 2021, 1:42 PM
ymandel added inline comments.Oct 6 2021, 1:43 PM
clang/docs/LibASTMatchersReference.html
636

Any luck fixing these edits?

jcking1034 updated this revision to Diff 377845.Oct 7 2021, 7:41 AM

Update matchers reference page.

jcking1034 updated this revision to Diff 377854.Oct 7 2021, 8:05 AM

Fix documentation for hasTemplateArgumentLoc.

aaron.ballman added inline comments.Oct 7 2021, 8:29 AM
clang/include/clang/ASTMatchers/ASTMatchers.h
6361–6362

I'm trying to reason my way through this. You want to match a qualified type location and int * const matches that. Then you want it to match an unqualified type loc and int matches that. Then it should be a pointer type... but int does not match that. So I wouldn't expect x to be matched. What have I misunderstood?

6406–6414

I'd appreciate more documentation on whether this is expected to match both lvalue and rvalue references. I suppose there's a secondary question of whether this matches member functions too:

struct S {
  void func() &; // Can this match this as a reference type loc?
};
6469–6487

Should these live in ASTMatchersInternal.h?

6510–6520

It'd be helpful to document how this behaves in C where the tag needs to be used (unlike in C++ where the tag can be elided unless you want an elaborated type). Same below.

jcking1034 updated this revision to Diff 377885.Oct 7 2021, 9:14 AM

Actually fix documentation for hasTemplateArgumentLoc.

jcking1034 marked 2 inline comments as done.

Address comments.

jcking1034 marked an inline comment as not done.Oct 7 2021, 11:49 AM
jcking1034 added inline comments.
clang/docs/LibASTMatchersReference.html
636

I think I got it to work!

clang/include/clang/ASTMatchers/ASTMatchers.h
6361–6362

The hasUnqualifiedLoc will actually be matched by int * (the * is part of the unqualified TypeLoc), which will then be a pointer TypeLoc that can be matched by pointerTypeLoc. Thus, the TypeLoc of x should be matched.

I just realized that the unit test BindsToConstVolatilePointerVarDecl which covers a similar situation is broken, where the match is producing a false positive (probably matching some hidden boiler plate code) - I've gone ahead and fixed the test, which hopefully clarifies things. I'll also add a unit test for these cases, as well.

6406–6414

I've added an example to clarify that this matches both lvalue and rvalue references. Having some trouble addressing your second point, but will keep playing around with it.

6510–6520

This matcher should match in C, I've updated the comment to reflect this. I've also updated the unit test ElaboratedTypeLocTest_BindsToElaboratedStructDeclaration to cover both the C and C++ cases.

Aside from the question about member function references and whether those should or should not be matched, I think this LGTM.

clang/include/clang/ASTMatchers/ASTMatchers.h
6361–6362

Ah, okay, I see where my confusion came in then. Thank you for the explanation and the additional unit test.

jcking1034 added inline comments.Oct 7 2021, 3:04 PM
clang/include/clang/ASTMatchers/ASTMatchers.h
6406–6414

After looking into it, it seems that this matcher will not match ref-qualified member functions. Essentially, this node isn't represented in a way that allows for it to be matched by referenceTypeLoc.

For a more detailed explaination: In this example, we are able to use cxxMethodDecl to match void func() &. If we wished to match the Type of the member function, we could use cxxMethodDecl(hasType(functionProtoType().bind("t"))), and from the FunctionProtoType node you could determine if the function is ref-qualified through a call to getRefQualifier (https://clang.llvm.org/doxygen/classclang_1_1FunctionProtoType.html). It's not possible to match using cxxMethodDecl(hasType(referenceType().bind("t"))). So it seems that the type of the member function is not a reference type, but instead a FunctionProtoType from which you'd programmatically have to determine if it's a reference type. In the realm of TypeLocs, there is no TypeLoc that corresponds to the FunctionProtoType node, and so the best you could do may be something like cxxMethodDecl(hasTypeLoc(loc(functionProtoType().bind("t")))) and programmatically analyzing the bound node. For reference, please see https://godbolt.org/z/qxsEb6a5Y.

aaron.ballman accepted this revision.Oct 8 2021, 6:34 AM

LGTM!

clang/include/clang/ASTMatchers/ASTMatchers.h
6406–6414

Thank you for the detailed explanation! I think the current behavior is fine.

Rebase onto master

This revision was landed with ongoing or failed builds.Oct 8 2021, 10:43 AM
Closed by commit rGac7429656286: Add `TypeLoc`-related matchers. (authored by jcking1034, committed by ymandel). · Explain Why
This revision was automatically updated to reflect the committed changes.

Oh! I did think of something else we should do -- can you add a release note to make users aware of the awesome new functionality? It doesn't have to be super detailed, but enough to alert people to the fact that this is significant new functionality.

jcking1034 added a comment.EditedOct 8 2021, 2:13 PM

@aaron.ballman that sounds like a good idea, what's the process for adding a release note? Does it involve editing clang/docs/ReleaseNotes.rst?

@aaron.ballman that sounds like a good idea, what's the process for adding a release note? Does it involve editing clang/docs/ReleaseNotes.rst?

That's correct! And because it's not a functionality change, you don't need to get a review before committing those changes unless you'd like the review (which is also totally fine). I'd add these details under the AST Matchers section of the document.