[PATCH] D124351: [Clang] Implement Change scope of lambda trailing-return-type

[Corentin Jabot via Phabricator via cfe-commits]

cor3ntin added a comment.

In D124351#4207501 <https://reviews.llvm.org/D124351#4207501>, @eandrews wrote:

> This patch causes an assertion when the attribute argument is an integer constant expression - https://godbolt.org/z/osKx5ejMb and has resulted in test fails downstream since any attribute which uses `VerifyIntegerConstantExpression` now hits this assert if used with lambdas.
>
> The assert hit is in `getCurLambda()` :
>
>   auto *CurLSI = dyn_cast<LambdaScopeInfo>(*I);
>    if (CurLSI && CurLSI->Lambda && CurLSI->CallOperator &&
>        !CurLSI->Lambda->Encloses(CurContext) && CurLSI->AfterParameterList) {
>      // We have switched contexts due to template instantiation.
>      assert(!CodeSynthesisContexts.empty()); <----- Hits this
>      return nullptr;
>    }
>
> Prior to this patch, Lambda Scope Information was not populated till after semantic analysis of attributes. This meant `CurLSI->Lambda` returned nullptr and we never entered the `if`. This patch however populates LSI during semantic analysis of lambda expression after introducer, which means we now enter the `if` during semantic analysis of the attribute` (`AfterParameterList` will be true at this point since this assert is hit way past parsing the parameters. `CurContext` is `foo`. I don't know if `CurContext` is right/wrong without further debugging.)
>
> For the godbolt test pasted above, neither before nor after this patch do we hit the code where `CodeSynthesisContexts` is populated. I wanted to see what code actually enters that block and so I tried deleting the code to see what tests fails. What is interesting is that nothing does :/ So I don't know if that means our tests are incomplete or if this code is not required.

Thanks :)
As far i could tell, this check is only there to catch developer mistakes. We could probably hide it under #ifndef NDEBUG - seems preferable than to remove it entirely - and then make sure AfterParameterList is appropriately set to false when parsing GNU attributes. I might have time to look into it later today if you don't.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124351