[PATCH] D122121: [clang][dataflow] Add action caching support to MatchSwitch

Mon Mar 21 08:59:31 PDT 2022

xazax.hun accepted this revision.
xazax.hun added a comment.

Thanks! Did you have a chance whether this makes a difference in real world scenarios? I'm mostly curious because I do not have a good mental model of how the matchers are implemented, specifically what optimizations are in place, so I don't really know how much of an impact can caching make :)

Once the rest of the comments are resolved this looks good to me.

In D122121#3396149 <https://reviews.llvm.org/D122121#3396149>, @ymandel wrote:

> In D122121#3396096 <https://reviews.llvm.org/D122121#3396096>, @sgatev wrote:
>
>> I was thinking that maybe we can split the problem in two:
>>
>> 1. duplicated work across `matchDynamic` calls, and
>> 2. duplicated work within a single `matchDynamic` call.
>>
>> This patch addresses 1. as I see it as responsibility of `MatchSwitch`. Your comment seems to be about 2., if I'm reading it correctly.
>
> Right. This division makes sense, particularly the point about responsibility. Point 2 is very much independent of `MatchSwitch`. But, I think matching the `optional` type is about both really in that every time we call the matcher we'll be checking the optional type, even if we only check it once *within* a single match. Still, fixing it doen't belong in `MatchSwitch`.

+1, I like how the problem was split. I also agree that `MatchSwitch` might not be the best place to fix #2 as a more broad fix would potentially benefit other parts of Clang, like Tidy.  But speaking of other parts of Clang, I wonder whether `MatchSwitch` is something that could be part of the ASTMatcher library. Some people might find it useful for implementing tools other than flow sensitive checks. Although, I do not have a strong preference, I'm completely fine leaving it where it currently is.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122121