[PATCH] D135690: [ASTMatchers] Add matcher for functions that are effectively inline

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D135690#3858621 <https://reviews.llvm.org/D135690#3858621>, @Trass3r wrote:

> In D135690#3858541 <https://reviews.llvm.org/D135690#3858541>, @aaron.ballman wrote:
>
>> In D135690#3856863 <https://reviews.llvm.org/D135690#3856863>, @Trass3r wrote:
>>
>>> Didn't realize it has a big cost. Looking inside the `AST_MATCHER` and `REGISTER_MATCHER` macros I can't see any unique instantiations, should be memoized?
>>
>> IIRC, the cost may depend on the compiler. I know we had to enable `/bigobj` when building with MSVC because each template instantiation here was being added to its own section in the object file and we'd wind up with tens of thousands of sections.
>
> Interesting, was that before/without -Zc:inline <https://learn.microsoft.com/en-us/cpp/build/reference/zc-inline-remove-unreferenced-comdat>?

Pretty sure that was after `/Zc:inline` (IIRC that existed in MSVC 2015 which was roughly when I recall we were hitting this).

>> I think it might make more sense to use a local matcher if you need it only for one clang-tidy check. If we find we need it in more checks or there's a wider need for it, we can hoist it up to ASTMatchers.h so it's exposed more generally at that time. WDYT?
>
> Yeah a quick search revealed 2 places where it might be useful:
> https://github.com/llvm/llvm-project/blob/b6c6933e9548f17d567a20c83eede16b3fcb0c2b/clang-tools-extra/clang-tidy/misc/DefinitionsInHeadersCheck.cpp#L100-L101
> https://github.com/llvm/llvm-project/blob/22e4203df813a8051b40adb3e2872e30fdbe1bbe/clang-tools-extra/clang-move/Move.cpp#L243-L245
>
> Of course as shown there you can always do additional checks after matching when writing a check in C++.
> But for tools like `clang-query` the matcher might be useful. Maybe there are also similar code search tools out there based on matchers?

The trouble with clang-query is that it can be used as a reason to expose *any* matcher, because it's specifically about exploring how to match the AST. I do know there are folks who use search tools based on matchers that aren't clang-tidy, but we don't usually support those as part of the matcher interface without a compelling reason.

The two examples you found do look like plausible reasons to expose this matcher though. Would you mind proving that out by switching those over to use the new matcher (as a second patch building on top of this one)? If it looks like we can easily use the new matcher in those checks, then I think it's a good enough reason to add the matcher (and update those checks).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D135690/new/

https://reviews.llvm.org/D135690