[clang] [Sema] Fix computations of "unexpanded packs" in substituted lambdas (PR #99882)

[Ilya Biryukov via cfe-commits]

ilya-biryukov wrote:

> I think I prefer the approach chosen by @zyn0217.
> 
> The changes are more targeted, and using a visitor for the call operator avoid a lot of code duplication. In particular, if we want to properly handle attributes, we might need code in `CollectUnexpandedParameterPacksVisitor.` Or maybe we just need a generic solution to store a flag on attributes.

The trick is that `CollectUnexpandedParameterPacksVisitor` already handles the attributes correctly (because `RecursiveASTVisitor` does that), but the `ContainsUnexpandedParameterPack` flag is not updated correctly on template instantiations, because it's not stored in the `Attr`.
I'm happy to write a patch that adds it to `Attr`, I believe we have free bits there without increasing the Attr's size and the work is not intersecting with @zyn0217's approach in any way and would just be a general improvement.

> I'd also would like us to avoid going over the lambda multiple times if we don't need to.
Do you worry about runtime performance?
The runtime overhead should be negligible there, note that we do not process anything recursively. It does seem like a lot of code, but all accesses are to data readily available and won't ever go beyond a few pointer reads.  

I still feel that having the code computing unexpanded parameters scattered across the codebase and duplicated between parsing and tree-transform/template-instantiations is a bigger price to pay in terms of complexity and readability than the runtime costs here.
I am still happy to go with the other patch eventually, just wanted to give it another go, especially given that @zyn0217 mentioned that avoiding scattering the code across the codebase is a good idea too, see what @zyn0217 said in the previous comment:
> it will tidy up things like how ContainsUnexpandedPacks are propagated and could avoid scattered flag setting

> I would suggest:
> 
> * Make a separate PR for template default arguments

I've already sent it out in #99880. It has no tests, though, because I could not find another way to dump these or trigger, but the changes are quite obvious and you'll get tests from either of the PRs anyway.

> * Make an NFC commit for the fold expression assert

Will do and rebase.

> * Help @zyn0217 ensure his PR covers all the failing tests

Sure, I am happy to either have the tests included here copied into #86265 or send follow up fixes covering anything that won't be included for whatever reason.

> * Find a good solution for attributes. If you tell me that there is a business reason to get `diagnose_if` working in 19, i think an ad-hoc solution is fine, as long as we replace it soon.

As mentioned above, I'm happy to write a patch for this as it seems relatively independent. Will post it when it's ready.

> Does that seem reasonable?

Yes, absolutely, it's a very reasonable option. I just wanted to reiterate that the costs of having the code handling unexpanded packs scattered and duplicated around parsing and template instantiations do seem high to me.
However, I'm happy to concede to the other approach if you and other maintainers feel that's the right way to go.

https://github.com/llvm/llvm-project/pull/99882