This is an archive of the discontinued LLVM Phabricator instance.

Add `withIntrospection` matcher.
Needs ReviewPublic

Authored by jcking1034 on Nov 15 2021, 2:29 PM.

Details

Summary

Adds a withIntrospection matcher which outputs contextual information for
better debugging. The format of the withIntrospection matcher's output is
open to debate.

This revision is intended for review, but also serves to demonstrate a use
case of the changes made in https://reviews.llvm.org/D113917, as these changes
allow for the withTag matcher to access the names of matchers.

Diff Detail

Event Timeline

jcking1034 requested review of this revision.Nov 15 2021, 2:29 PM
jcking1034 created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptNov 15 2021, 2:29 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
jcking1034 retitled this revision from Add withTag matcher. to Add `withTag` matcher..Nov 15 2021, 2:33 PM
jcking1034 edited the summary of this revision. (Show Details)Nov 15 2021, 2:43 PM
jcking1034 edited the summary of this revision. (Show Details)Nov 15 2021, 2:47 PM
jcking1034 edited the summary of this revision. (Show Details)
jcking1034 edited the summary of this revision. (Show Details)
jcking1034 edited the summary of this revision. (Show Details)Nov 15 2021, 3:11 PM

Improve wrapping of PolymorphicMatchers

Change getMatcherName to getName

Chang withTag to withIntrospection

jcking1034 retitled this revision from Add `withTag` matcher. to Add `withIntrospection` matcher..Nov 29 2021, 11:57 AM
jcking1034 edited the summary of this revision. (Show Details)

Nit:
Personally, I would prefer the name withDebugOutput over withIntrospection, as it more clearly describes the intent of this matcher.
At least to me, "introspection" is usually something done programatically, i.e. the matcher somehow reflects about its own state, but in this case all we are doing is printing some debug output to stderr.

Code looks good to me, but I don't know the LLVM coding guidelines and hence can't provide an actual code review

clang/include/clang/ASTMatchers/ASTMatchers.h
3000–3001

std::move(BeforeTag), std::move(AfterTag)

No need to create additional copies of those strings. Here and other places

Nit:
Personally, I would prefer the name withDebugOutput over withIntrospection, as it more clearly describes the intent of this matcher.
At least to me, "introspection" is usually something done programatically, i.e. the matcher somehow reflects about its own state, but in this case all we are doing is printing some debug output to stderr.

FWIW, I tend to agree that "introspection" is a bit of an odd choice for naming here. withDebugOutput would be an improvement, but withPrefixAndSuffix or something along those lines would also work.

clang/include/clang/ASTMatchers/ASTMatchers.h
3000–3001

Alternatively, it'd be great if we can switch these to use StringRef to avoid copies.

clang/include/clang/ASTMatchers/ASTMatchersInternal.h
1064–1066

All of those identifiers introduce UB because they're reserved, dropping the underscore solves the issue (and, oddly, matches our terrible coding style where we let ctor parameters shadow the names of member variables).

1784