Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Clang C++20 Feature: P0588R1 - Simplifying implicit lambda capture #61426

Open
h-vetinari opened this issue Mar 15, 2023 · 4 comments
Open

Clang C++20 Feature: P0588R1 - Simplifying implicit lambda capture #61426

h-vetinari opened this issue Mar 15, 2023 · 4 comments
Assignees
@LYP951018
Labels
c++20 clang:frontend Language frontend issues, e.g. anything involving "Sema" defect report Issues representing WG21 papers to be applied as a defect report

Comments

@h-vetinari
Copy link
Contributor

id: P0588R1
paper: https://wg21.link/P0588R1

This paper doesn't have a tracking issue in the C++20 project yet1. It also shows up on the cppreference implementation status page, and presumably should be tracked in https://clang.llvm.org/cxx_status.html (like other DRs that have a Pxxxx).

I found some related issues2, but none dedicated to this:

Most recently, @erichkeane wrote in #58872:

DR status is tracked here : https://clang.llvm.org/cxx_dr_status.html

So it would be tracked there as CWG1632 and CWG1913.

From the looks of it, implementation will likely quickly get into a "thar be dragons" part of the compiler. We need to suppress instantiation of the body of dependent lambdas (the easy part), but getInstantiationArgs likely needs updating for it (which is a function that had some serious problems) to properly handle this and other cases.

This was followed by https://reviews.llvm.org/D138148 (open) & https://reviews.llvm.org/D137244 (merged). In the latter, @cor3ntin notes:

Either way, I think this makes support for P0588 complete, but we probably want to add a test for that in a separate PR.
(and I lack confidence I understand P0588 sufficiently to assert the completeness of our conformance).

Footnotes

  1. It's obviously questionable to call this a "C++20 Feature", but I'm following the pattern of the other paper-tracking issues. ↩

  2. curiously, GH search found nothing for "P0588", but did find something for "P0588R1" ↩
@EugeneZelenko EugeneZelenko added c++20 clang:frontend Language frontend issues, e.g. anything involving "Sema" and removed new issue labels Mar 15, 2023
@llvmbot
Copy link
Member

llvmbot commented Mar 15, 2023

@llvm/issue-subscribers-clang-frontend

@llvmbot
Copy link
Member

llvmbot commented Mar 15, 2023

@llvm/issue-subscribers-c-20

@frederick-vs-ja
Copy link
Contributor

  1. It's obviously questionable to call this a "C++20 Feature", but I'm following the pattern of the other paper-tracking issues.

I think it'd be better to add a "C++ Defect report" category, and avoid mentioning the revision by default.

@h-vetinari
Copy link
Contributor Author

and presumably should be tracked in https://clang.llvm.org/cxx_status.html (like other DRs that have a Pxxxx)

Actually, it is tracked, but under C++11 (which the CTRL+F search didn't find because it's hidden behind a fold-out; would be better to sort this in reverse chronological order and avoid the foldout IMO).

It seems there's a lack of consistency (both here and on https://en.cppreference.com/w/cpp/compiler_support) in which standard a DR gets noted.

@frederick-vs-ja: I think it'd be better to add a "C++ Defect report" category, and avoid mentioning the revision by default.

The problem is that there's already Defect Report status page, but with thousands of such reports, it's not going to be used other than for very specific lookup. So it might be worth to have a separate "Defect report" category on the feature page after all, for those DRs that come with separate papers (resp. are listed on cppreference).

@LYP951018 LYP951018 self-assigned this Mar 5, 2024
This was referenced Jul 6, 2024
Open
Merged
LYP951018 added a commit that referenced this issue Jul 9, 2024
@LYP951018
[Clang] Fix the order of addInstantiatedParameters in LambdaScopeForC…
f0c7505 
…allOperatorInstantiationRAII (#97215)

Currently, `addInstantiatedParameters` is called from the innermost
lambda outward. However, when the function parameters of an inner lambda
depend on the function parameters of an outer lambda, it can lead to a
crash due to the inability to find a mapping for the instantiated decl.

This PR corrects this behavior by calling `addInstantiatedParameters`
from the outside in.

repro code: https://godbolt.org/z/KbsxWesW6

```cpp
namespace dependent_param_concept {
template <typename... Ts> void sink(Ts...) {}
void dependent_param() {
  auto L = [](auto... x) {
    return [](decltype(x)... y) { // `y` depends on `x`
      return [](int z)
        requires requires { sink(y..., z); }
      {};
    };
  };
  L(0, 1)(1, 2)(1);
}
} // namespace dependent_param_concept
```

This PR is a prerequisite for implmenting #61426
aaryanshukla pushed a commit to aaryanshukla/llvm-project that referenced this issue Jul 14, 2024
@LYP951018 @aaryanshukla
[Clang] Fix the order of addInstantiatedParameters in LambdaScopeForC…
70cd3ed 
…allOperatorInstantiationRAII (llvm#97215)

Currently, `addInstantiatedParameters` is called from the innermost
lambda outward. However, when the function parameters of an inner lambda
depend on the function parameters of an outer lambda, it can lead to a
crash due to the inability to find a mapping for the instantiated decl.

This PR corrects this behavior by calling `addInstantiatedParameters`
from the outside in.

repro code: https://godbolt.org/z/KbsxWesW6

```cpp
namespace dependent_param_concept {
template <typename... Ts> void sink(Ts...) {}
void dependent_param() {
  auto L = [](auto... x) {
    return [](decltype(x)... y) { // `y` depends on `x`
      return [](int z)
        requires requires { sink(y..., z); }
      {};
    };
  };
  L(0, 1)(1, 2)(1);
}
} // namespace dependent_param_concept
```

This PR is a prerequisite for implmenting llvm#61426
@LYP951018 LYP951018 mentioned this issue Jul 26, 2024
Merged
LYP951018 added a commit that referenced this issue Aug 9, 2024
@LYP951018
[Clang] Fix Handling of Init Capture with Parameter Packs in LambdaSc…
52126dc 
…opeForCallOperatorInstantiationRAII (#100766)

This PR addresses issues related to the handling of `init capture` with
parameter packs in Clang's
`LambdaScopeForCallOperatorInstantiationRAII`.

Previously, `addInstantiatedCapturesToScope` would add `init capture`
containing packs to the scope using the type of the `init capture` to
determine the expanded pack size. However, this approach resulted in a
pack size of 0 because `getType()->containsUnexpandedParameterPack()`
returns `false`. After extensive testing, it appears that the correct
pack size can only be inferred from `getInit`.

But `getInit` may reference parameters and `init capture` from an outer
lambda, as shown in the following example:

```cpp
auto L = [](auto... z) {
    return [... w = z](auto... y) {
        // ...
    };
};
```

To address this, `addInstantiatedCapturesToScope` in
`LambdaScopeForCallOperatorInstantiationRAII` should be called last.
Additionally, `addInstantiatedCapturesToScope` has been modified to only
add `init capture` to the scope. The previous implementation incorrectly
called `MakeInstantiatedLocalArgPack` for other non-init captures
containing packs, resulting in a pack size of 0.

### Impact

This patch affects scenarios where
`LambdaScopeForCallOperatorInstantiationRAII` is passed with
`ShouldAddDeclsFromParentScope = false`, preventing the correct addition
of the current lambda's `init capture` to the scope. There are two main
scenarios for `ShouldAddDeclsFromParentScope = false`:

1. **Constraints**: Sometimes constraints are instantiated in place
rather than delayed. In this case,
`LambdaScopeForCallOperatorInstantiationRAII` does not need to add `init
capture` to the scope.
2. **`noexcept` Expressions**: The expressions inside `noexcept` have
already been transformed, and the packs referenced within have been
expanded. Only `RebuildLambdaInfo` needs to add the expanded captures to
the scope, without requiring `addInstantiatedCapturesToScope` from
`LambdaScopeForCallOperatorInstantiationRAII`.

### Considerations

An alternative approach could involve adding a data structure within the
lambda to record the expanded size of the `init capture` pack. However,
this would increase the lambda's size and require extensive
modifications.

This PR is a prerequisite for implmenting
#61426
@LYP951018 LYP951018 mentioned this issue Aug 24, 2024
Draft
@frederick-vs-ja frederick-vs-ja added the defect report Issues representing WG21 papers to be applied as a defect report label Mar 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@LYP951018 LYP951018

Labels
c++20 clang:frontend Language frontend issues, e.g. anything involving "Sema" defect report Issues representing WG21 papers to be applied as a defect report
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@LYP951018 @EugeneZelenko @frederick-vs-ja @h-vetinari @llvmbot