Page MenuHomePhabricator

[clang] add parameter pack/pack expansion matchers
AbandonedPublic

Authored by upsj on Apr 25 2022, 3:37 AM.

Details

Summary

There is no (obvious to me) way to match pack expansions and parameter packs, so I added two matchers for them.

Diff Detail

Event Timeline

upsj created this revision.Apr 25 2022, 3:37 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 25 2022, 3:37 AM
upsj requested review of this revision.Apr 25 2022, 3:37 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 25 2022, 3:37 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
upsj updated this revision to Diff 425487.Apr 27 2022, 5:20 AM

formatting

Due to how expensive it is to instantiate all the templates from ASTMatchers.h, we typically don't add new matchers unless there's a need for them in-tree or they're expected to be generally useful to a number of out-of-tree matchers. Are you planning to make use of these matchers for a clang-tidy check you're working on or something along those lines? Or are they mostly to add missing coverage?

On the technical side (feel free to ignore this until we know we're going to add the matchers to Clang if you'd like), you should add test coverage for the new matchers to clang/unittests/ASTMatchers and you should regenerate the documentation by running clang/docs/tools/dump_ast_matchers.py.

upsj added a comment.Apr 27 2022, 12:28 PM

This is part of my long-term goal to add support for forwarding parameters and documentation for make_unique-like functions to clangd. To be fair, the details are not entirely fleshed out - for inlay hints (displaying parameter names inline), the entire function template is already available in an instantiated form, so I can work with existing matchers. For signature help (displaying which parameters are available, which one is active while typing), IIRC you only have the template instantiation pattern available, so I need to find the necessary parts of the AST myself.

My thought was to try and move towards this goal in small steps, but if this is an important build time/size consideration, I can also see whether both of them are actually necessary (I suspect isParameterPack might not be) before moving forward?

This is part of my long-term goal to add support for forwarding parameters and documentation for make_unique-like functions to clangd. To be fair, the details are not entirely fleshed out - for inlay hints (displaying parameter names inline), the entire function template is already available in an instantiated form, so I can work with existing matchers. For signature help (displaying which parameters are available, which one is active while typing), IIRC you only have the template instantiation pattern available, so I need to find the necessary parts of the AST myself.

Ah, thank you for the background information!

My thought was to try and move towards this goal in small steps, but if this is an important build time/size consideration, I can also see whether both of them are actually necessary (I suspect isParameterPack might not be) before moving forward?

We support locally defined matchers so that you can still use all the AST matching awesomeness, but without increasing the compile times for everyone for one-off matchers. e.g,., https://github.com/llvm/llvm-project/blob/c874dd53628db8170d4c5ba3878817abc385a695/clang-tools-extra/clang-tidy/readability/ConvertMemberFunctionsToStatic.cpp#L23 My recommendation is to do something similar for clangd, but once we find a second use for the matcher, we can go ahead and hoist it into ASTMatchers.h at that point. WDYT?

upsj planned changes to this revision.Apr 27 2022, 12:50 PM

I wasn't aware of that, sounds perfect, thanks! Then I'll put this on hold until I figure out if it's really necessary.

upsj abandoned this revision.May 1 2022, 12:42 AM

I think we should be able to do this without adding new matchers globally.