[clang] [Clang][P1061] Add stuctured binding packs (PR #121417)
AaronBallman wrote: > However, we should target to have it landed before rc2 or something like that > We'd make a lot of folks happy having that in 20, I think. And while the PR > is not tiny, it's seems also fairly low risk (famous last words!) I'm not comfortable pushing large, new features into the release candidates without some significant justification. There's another release coming in six months and having time to ensure features are fully baked is important (for example, this hasn't had time to wind its way through several of the downstreams who do significant testing on internal test corpuses). So personally, I'd prefer to wait unless this is a significant blocker for something else. https://github.com/llvm/llvm-project/pull/121417 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][P1061] Add stuctured binding packs (PR #121417)
erichkeane wrote: > The code and test coverage looks good. There are a couple of comments that > have not been addressed. > > And there is @zygoloid suggestion to generalize > `SubstNonTypeTemplateParmPackExpr` - do we want to do that now or address it > after the fact? > > Maybe trying to land that today is not the brightest idea. However, we should > target to have it landed before rc2 or something like that We'd make a lot of > folks happy having that in 20, I think. And while the PR is not tiny, it's > seems also fairly low risk (famous last words!) > > @erichkeane @AaronBallman @Sirraide Yeah, I think I agree. I don't see much risk here besides this feature being broken, which I'm OK with. https://github.com/llvm/llvm-project/pull/121417 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][P1061] Add stuctured binding packs (PR #121417)
llvm-ci wrote: LLVM Buildbot has detected a new failure on builder `openmp-offload-amdgpu-runtime` running on `omp-vega20-0` while building `clang` at step 7 "Add check check-offload". Full details are available at: https://lab.llvm.org/buildbot/#/builders/30/builds/14879 Here is the relevant piece of the build log for the reference ``` Step 7 (Add check check-offload) failure: test (failure) ... PASS: libomptarget :: x86_64-unknown-linux-gnu-LTO :: offloading/bug47654.cpp (996 of 1005) PASS: libomptarget :: x86_64-unknown-linux-gnu-LTO :: offloading/bug50022.cpp (997 of 1005) PASS: libomptarget :: x86_64-unknown-linux-gnu-LTO :: offloading/test_libc.cpp (998 of 1005) PASS: libomptarget :: x86_64-unknown-linux-gnu-LTO :: offloading/wtime.c (999 of 1005) PASS: libomptarget :: x86_64-unknown-linux-gnu :: offloading/bug49021.cpp (1000 of 1005) PASS: libomptarget :: x86_64-unknown-linux-gnu :: offloading/std_complex_arithmetic.cpp (1001 of 1005) PASS: libomptarget :: x86_64-unknown-linux-gnu-LTO :: offloading/complex_reduction.cpp (1002 of 1005) PASS: libomptarget :: x86_64-unknown-linux-gnu-LTO :: offloading/bug49021.cpp (1003 of 1005) PASS: libomptarget :: x86_64-unknown-linux-gnu-LTO :: offloading/std_complex_arithmetic.cpp (1004 of 1005) TIMEOUT: libomptarget :: amdgcn-amd-amdhsa :: offloading/parallel_offloading_map.cpp (1005 of 1005) TEST 'libomptarget :: amdgcn-amd-amdhsa :: offloading/parallel_offloading_map.cpp' FAILED Exit Code: -9 Timeout: Reached timeout of 100 seconds Command Output (stdout): -- # RUN: at line 1 /home/ompworker/bbot/openmp-offload-amdgpu-runtime/llvm.build/./bin/clang++ -fopenmp-I /home/ompworker/bbot/openmp-offload-amdgpu-runtime/llvm.src/offload/test -I /home/ompworker/bbot/openmp-offload-amdgpu-runtime/llvm.build/runtimes/runtimes-bins/openmp/runtime/src -L /home/ompworker/bbot/openmp-offload-amdgpu-runtime/llvm.build/runtimes/runtimes-bins/offload -L /home/ompworker/bbot/openmp-offload-amdgpu-runtime/llvm.build/./lib -L /home/ompworker/bbot/openmp-offload-amdgpu-runtime/llvm.build/runtimes/runtimes-bins/openmp/runtime/src -nogpulib -Wl,-rpath,/home/ompworker/bbot/openmp-offload-amdgpu-runtime/llvm.build/runtimes/runtimes-bins/offload -Wl,-rpath,/home/ompworker/bbot/openmp-offload-amdgpu-runtime/llvm.build/runtimes/runtimes-bins/openmp/runtime/src -Wl,-rpath,/home/ompworker/bbot/openmp-offload-amdgpu-runtime/llvm.build/./lib -fopenmp-targets=amdgcn-amd-amdhsa /home/ompworker/bbot/openmp-offload-amdgpu-runtime/llvm.src/offload/test/offloading/parallel_offloading_map.cpp -o /home/ompworker/bbot/openmp-offload-amdgpu-runtime/llvm.build/runtimes/runtimes-bins/offload/test/amdgcn-amd-amdhsa/offloading/Output/parallel_offloading_map.cpp.tmp /home/ompworker/bbot/openmp-offload-amdgpu-runtime/llvm.build/./lib/libomptarget.devicertl.a && /home/ompworker/bbot/openmp-offload-amdgpu-runtime/llvm.build/runtimes/runtimes-bins/offload/test/amdgcn-amd-amdhsa/offloading/Output/parallel_offloading_map.cpp.tmp | /home/ompworker/bbot/openmp-offload-amdgpu-runtime/llvm.build/./bin/FileCheck /home/ompworker/bbot/openmp-offload-amdgpu-runtime/llvm.src/offload/test/offloading/parallel_offloading_map.cpp # executed command: /home/ompworker/bbot/openmp-offload-amdgpu-runtime/llvm.build/./bin/clang++ -fopenmp -I /home/ompworker/bbot/openmp-offload-amdgpu-runtime/llvm.src/offload/test -I /home/ompworker/bbot/openmp-offload-amdgpu-runtime/llvm.build/runtimes/runtimes-bins/openmp/runtime/src -L /home/ompworker/bbot/openmp-offload-amdgpu-runtime/llvm.build/runtimes/runtimes-bins/offload -L /home/ompworker/bbot/openmp-offload-amdgpu-runtime/llvm.build/./lib -L /home/ompworker/bbot/openmp-offload-amdgpu-runtime/llvm.build/runtimes/runtimes-bins/openmp/runtime/src -nogpulib -Wl,-rpath,/home/ompworker/bbot/openmp-offload-amdgpu-runtime/llvm.build/runtimes/runtimes-bins/offload -Wl,-rpath,/home/ompworker/bbot/openmp-offload-amdgpu-runtime/llvm.build/runtimes/runtimes-bins/openmp/runtime/src -Wl,-rpath,/home/ompworker/bbot/openmp-offload-amdgpu-runtime/llvm.build/./lib -fopenmp-targets=amdgcn-amd-amdhsa /home/ompworker/bbot/openmp-offload-amdgpu-runtime/llvm.src/offload/test/offloading/parallel_offloading_map.cpp -o /home/ompworker/bbot/openmp-offload-amdgpu-runtime/llvm.build/runtimes/runtimes-bins/offload/test/amdgcn-amd-amdhsa/offloading/Output/parallel_offloading_map.cpp.tmp /home/ompworker/bbot/openmp-offload-amdgpu-runtime/llvm.build/./lib/libomptarget.devicertl.a # note: command had no output on stdout or stderr # executed command: /home/ompworker/bbot/openmp-offload-amdgpu-runtime/llvm.build/runtimes/runtimes-bins/offload/test/amdgcn-amd-amdhsa/offloading/Output/parallel_offloading_map.cpp.tmp # note: command had no output on stdout or stderr # error: command failed with exit status: -9 # error: command reached timeout: True # executed command: /home/ompworker/bbot/openmp-offload-amdgpu-runtime/
[clang] [Clang][P1061] Add stuctured binding packs (PR #121417)
https://github.com/cor3ntin closed https://github.com/llvm/llvm-project/pull/121417 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][P1061] Add stuctured binding packs (PR #121417)
cor3ntin wrote: > > Does this mean we should adjust the cxx_status page to not say Clang 20? I > > also see there is a conflict with the ReleaseNotes.md where it appears all > > of the C++2c items no longer exist. (I guess that is because it is for the > > next release.) > > Meh, let's assume 20 and we can edit it later if we end up not merging, I > think. And yes, the release notes have been reset for 21, I guess you can > remove the entry entirely to make it easier to cherry-pick to 20 Edit: On second thought, lets revert cxx_status.html too to avoid confusion https://github.com/llvm/llvm-project/pull/121417 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][P1061] Add stuctured binding packs (PR #121417)
ricejasonf wrote: >@ricejasonf let us know if you want us to merge on your behalf @cor3ntin When you think its ready, yes please. I think it requires write access. I fixed the conflict in the ReleaseNotes through Github's UI so that only my feature is under the C++2c features added. I assume other ones were removed as they are for the "previous release". https://github.com/llvm/llvm-project/pull/121417 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][P1061] Add stuctured binding packs (PR #121417)
cor3ntin wrote: > Does this mean we should adjust the cxx_status page to not say Clang 20? I > also see there is a conflict with the ReleaseNotes.md where it appears all of > the C++2c items no longer exist. (I guess that is because it is for the next > release.) Meh, let's assume 20 and we can edit it later if we end up not merging, I think. And yes, the release notes have been reset for 21, I guess you can remove the entry entirely to make it easier to cherry-pick to 20 https://github.com/llvm/llvm-project/pull/121417 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][P1061] Add stuctured binding packs (PR #121417)
ricejasonf wrote: @cor3ntin > Now that the Clang 20 branch has happened, I think we should go ahead with > that PR Does this mean we should adjust the cxx_status page to not say Clang 20? I also see there is a conflict with the ReleaseNotes.md where it appears all of the C++2c items no longer exist. (I guess that is because it is for the next release.) > It would be great to explore merging ResolvedUnexpandedPackExpr and > FunctionParmPackExpr in a separate PR. Is that something you would be > interested in exploring? I can look into this. I will consider `SubstNonTypeTemplateParmPackExpr` as well although I am not sure how special that one is. >Thank you Jason for contributing this feature and for being reactive on this >rather involved review :) Likewise and thank you and the others for your contributions and diligence while reviewing this! https://github.com/llvm/llvm-project/pull/121417 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][P1061] Add stuctured binding packs (PR #121417)
cor3ntin wrote: @ricejasonf let us know if you want us to merge on your behalf https://github.com/llvm/llvm-project/pull/121417 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][P1061] Add stuctured binding packs (PR #121417)
https://github.com/cor3ntin approved this pull request. Now that the Clang 20 branch has happened, I think we should go ahead with that PR - It would be great to explore merging ResolvedUnexpandedPackExpr and FunctionParmPackExpr in a separate PR. Is that something you would be interested in exploring? - If there isn't any issue reported in the next ~ 2 weeks or so we should backport that to the Clang 20 branch Thank you Jason for contributing this feature and for being reactive on this rather involved review :) https://github.com/llvm/llvm-project/pull/121417 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][P1061] Add stuctured binding packs (PR #121417)
@@ -4213,8 +4226,35 @@ class DecompositionDecl final
static DecompositionDecl *CreateDeserialized(ASTContext &C, GlobalDeclID ID,
unsigned NumBindings);
- ArrayRef bindings() const {
-return llvm::ArrayRef(getTrailingObjects(), NumBindings);
+ // Provide the range of bindings which may have a nested pack.
+ llvm::ArrayRef bindings() const {
+return {getTrailingObjects(), NumBindings};
+ }
+
+ // Provide a flattened range to visit each binding.
+ auto flat_bindings() const {
+llvm::ArrayRef Bindings = bindings();
+llvm::ArrayRef PackExprs;
+
+// Split the bindings into subranges split by the pack.
+auto S1 = Bindings.take_until(
+[](BindingDecl *BD) { return BD->isParameterPack(); });
+
+Bindings = Bindings.drop_front(S1.size());
+if (!Bindings.empty()) {
+ PackExprs = Bindings.front()->getBindingPackExprs();
+ Bindings = Bindings.drop_front();
+}
+
+auto S2 = llvm::map_range(PackExprs, [](Expr *E) {
+ auto *DRE = cast(E);
+ return cast(DRE->getDecl());
+});
+
+// llvm::concat must take temporaries or it will capture
+// references.
ricejasonf wrote:
Ah, right. I just removed the comment.
https://github.com/llvm/llvm-project/pull/121417
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][P1061] Add stuctured binding packs (PR #121417)
https://github.com/cor3ntin commented: The code and test coverage looks good. There are a couple of comments that have not been addressed. And there is @zygoloid suggestion to generalize `SubstNonTypeTemplateParmPackExpr`. Maybe trying to land that today is not the brightest idea. However, we should target to have it landed before rc2 or something like that (we'd make a lot of folks happy having that in 20, I think) @erichkeane @AaronBallman @Sirraide https://github.com/llvm/llvm-project/pull/121417 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][P1061] Add stuctured binding packs (PR #121417)
@@ -5321,6 +5321,59 @@ class BuiltinBitCastExpr final } }; +// Represents an unexpanded pack where the list of expressions are +// known. These are used when structured bindings introduce a pack. +class ResolvedUnexpandedPackExpr final cor3ntin wrote: Should we try to address that? https://github.com/llvm/llvm-project/pull/121417 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][P1061] Add stuctured binding packs (PR #121417)
https://github.com/cor3ntin edited https://github.com/llvm/llvm-project/pull/121417 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][P1061] Add stuctured binding packs (PR #121417)
https://github.com/cor3ntin edited https://github.com/llvm/llvm-project/pull/121417 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][P1061] Add stuctured binding packs (PR #121417)
@@ -1099,6 +1099,13 @@ def err_lambda_capture_misplaced_ellipsis : Error< "the name of the capture">; def err_lambda_capture_multiple_ellipses : Error< "multiple ellipses in pack capture">; +def err_binding_multiple_ellipses : Error< + "multiple packs in structured binding declaration">; +def note_previous_ellipsis : Note< + "previous binding pack specified here">; +def ext_cxx_binding_pack : ExtWarn< cor3ntin wrote: This is not addressed yet https://github.com/llvm/llvm-project/pull/121417 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][P1061] Add stuctured binding packs (PR #121417)
https://github.com/cor3ntin edited https://github.com/llvm/llvm-project/pull/121417 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][P1061] Add stuctured binding packs (PR #121417)
@@ -1523,9 +1570,25 @@ void
Sema::CheckCompleteDecompositionDeclaration(DecompositionDecl *DD) {
// If the type of the decomposition is dependent, then so is the type of
// each binding.
if (DecompType->isDependentType()) {
-for (auto *B : DD->bindings())
- B->setType(Context.DependentTy);
+for (auto *B : DD->bindings()) {
+ // Do not overwrite any pack type.
+ if (B->getType().isNull())
+B->setType(Context.DependentTy);
+}
ricejasonf wrote:
The comment I updated is above this.
https://github.com/llvm/llvm-project/pull/121417
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][P1061] Add stuctured binding packs (PR #121417)
@@ -3423,6 +3425,13 @@ VarDecl *BindingDecl::getHoldingVar() const {
return VD;
}
+llvm::ArrayRef BindingDecl::getBindingPackExprs() const {
+ if (!Binding)
+return {};
ricejasonf wrote:
Added. The assert was being triggered in one spot that I realized was
unnecessary (when I tried removing the whole block of code that was going back
and adding types to the DeclRefExprs.)
https://github.com/llvm/llvm-project/pull/121417
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][P1061] Add stuctured binding packs (PR #121417)
https://github.com/ricejasonf edited https://github.com/llvm/llvm-project/pull/121417 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][P1061] Add stuctured binding packs (PR #121417)
@@ -1523,9 +1570,25 @@ void
Sema::CheckCompleteDecompositionDeclaration(DecompositionDecl *DD) {
// If the type of the decomposition is dependent, then so is the type of
// each binding.
if (DecompType->isDependentType()) {
-for (auto *B : DD->bindings())
- B->setType(Context.DependentTy);
+for (auto *B : DD->bindings()) {
+ // Do not overwrite any pack type.
+ if (B->getType().isNull())
+B->setType(Context.DependentTy);
+}
ricejasonf wrote:
This was here before. After processing the intializer the BindingDecls should
all have types set except in the case when the initializer is dependent. In
that case all the types are still Null so here we set them to dependent. My
change is just preventing it from overwriting the PackExpansionType if there is
one.
https://github.com/llvm/llvm-project/pull/121417
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][P1061] Add stuctured binding packs (PR #121417)
@@ -1523,9 +1570,25 @@ void
Sema::CheckCompleteDecompositionDeclaration(DecompositionDecl *DD) {
// If the type of the decomposition is dependent, then so is the type of
// each binding.
if (DecompType->isDependentType()) {
-for (auto *B : DD->bindings())
- B->setType(Context.DependentTy);
+for (auto *B : DD->bindings()) {
+ // Do not overwrite any pack type.
+ if (B->getType().isNull())
+B->setType(Context.DependentTy);
+}
cor3ntin wrote:
I really would like to understand when this happen. Maybe a longer comment is
in order?
https://github.com/llvm/llvm-project/pull/121417
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][P1061] Add stuctured binding packs (PR #121417)
@@ -3423,6 +3425,13 @@ VarDecl *BindingDecl::getHoldingVar() const {
return VD;
}
+llvm::ArrayRef BindingDecl::getBindingPackExprs() const {
+ if (!Binding)
+return {};
cor3ntin wrote:
Looking at how this is used, i think we can assert here instead
https://github.com/llvm/llvm-project/pull/121417
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][P1061] Add stuctured binding packs (PR #121417)
@@ -2484,6 +2489,16 @@ TemplateInstantiator::TransformDeclRefExpr(DeclRefExpr
*E) {
if (PD->isParameterPack())
return TransformFunctionParmPackRefExpr(E, PD);
+ if (BindingDecl *BD = dyn_cast(D); BD && BD->isParameterPack())
{
+BD = cast_or_null(TransformDecl(BD->getLocation(), BD));
+if (!BD)
+ return ExprError();
+if (auto *RP =
+dyn_cast_if_present(BD->getBinding()))
{
+ return TransformResolvedUnexpandedPackExpr(RP);
+}
+ }
+
cor3ntin wrote:
```suggestion
if (auto *RP =
dyn_cast_if_present(BD->getBinding()))
return TransformResolvedUnexpandedPackExpr(RP);
}
```
https://github.com/llvm/llvm-project/pull/121417
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][P1061] Add stuctured binding packs (PR #121417)
@@ -2670,8 +2670,11 @@ StmtResult Sema::BuildCXXForRangeStmt(
// them in properly when we instantiate the loop.
if (!LoopVar->isInvalidDecl() && Kind != BFRK_Check) {
if (auto *DD = dyn_cast(LoopVar))
-for (auto *Binding : DD->bindings())
- Binding->setType(Context.DependentTy);
+for (auto *Binding : DD->bindings()) {
+ if (!Binding->isParameterPack()) {
+Binding->setType(Context.DependentTy);
+ }
cor3ntin wrote:
```suggestion
if (!Binding->isParameterPack())
Binding->setType(Context.DependentTy);
```
https://github.com/llvm/llvm-project/pull/121417
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][P1061] Add stuctured binding packs (PR #121417)
https://github.com/cor3ntin edited https://github.com/llvm/llvm-project/pull/121417 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][P1061] Add stuctured binding packs (PR #121417)
https://github.com/cor3ntin commented: Some nitpicks. Getting close :) https://github.com/llvm/llvm-project/pull/121417 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][P1061] Add stuctured binding packs (PR #121417)
@@ -0,0 +1,117 @@ +// RUN: %clang_cc1 -fsyntax-only -std=c++26 %s -verify ricejasonf wrote: I added these to the bottom of this file. https://github.com/llvm/llvm-project/pull/121417 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][P1061] Add stuctured binding packs (PR #121417)
https://github.com/shafik edited https://github.com/llvm/llvm-project/pull/121417 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][P1061] Add stuctured binding packs (PR #121417)
https://github.com/shafik commented: Thank you for the PR, small comments. https://github.com/llvm/llvm-project/pull/121417 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][P1061] Add stuctured binding packs (PR #121417)
@@ -0,0 +1,11 @@
+// RUN: %clang_cc1 -std=c++2c -verify -fsyntax-only %s
+
+template
+void decompose_array() {
+ int arr[4] = {1, 2, 3, 5};
+ auto [x, ... // #1
+rest, ...more_rest] = arr; // expected-error{{multiple packs in structured
binding declaration}}
+ // expected-note@#1{{previous binding pack
specified here}}
+ //
+ auto [y...] = arr; // expected-error{{'...' must immediately precede
declared identifier}}
shafik wrote:
Maybe some tests w/ just `...` like
```cpp
auto [...] = arr; // error
auto [a,...,b] = arr; // error
```
https://github.com/llvm/llvm-project/pull/121417
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][P1061] Add stuctured binding packs (PR #121417)
@@ -0,0 +1,117 @@ +// RUN: %clang_cc1 -fsyntax-only -std=c++26 %s -verify shafik wrote: +1 we should always cover all the examples from the proposal. https://github.com/llvm/llvm-project/pull/121417 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][P1061] Add stuctured binding packs (PR #121417)
@@ -4213,8 +4226,35 @@ class DecompositionDecl final
static DecompositionDecl *CreateDeserialized(ASTContext &C, GlobalDeclID ID,
unsigned NumBindings);
- ArrayRef bindings() const {
-return llvm::ArrayRef(getTrailingObjects(), NumBindings);
+ // Provide the range of bindings which may have a nested pack.
+ llvm::ArrayRef bindings() const {
+return {getTrailingObjects(), NumBindings};
+ }
+
+ // Provide a flattened range to visit each binding.
+ auto flat_bindings() const {
+llvm::ArrayRef Bindings = bindings();
+llvm::ArrayRef PackExprs;
+
+// Split the bindings into subranges split by the pack.
+auto S1 = Bindings.take_until(
+[](BindingDecl *BD) { return BD->isParameterPack(); });
+
+Bindings = Bindings.drop_front(S1.size());
+if (!Bindings.empty()) {
+ PackExprs = Bindings.front()->getBindingPackExprs();
+ Bindings = Bindings.drop_front();
+}
+
+auto S2 = llvm::map_range(PackExprs, [](Expr *E) {
+ auto *DRE = cast(E);
+ return cast(DRE->getDecl());
+});
+
+// llvm::concat must take temporaries or it will capture
+// references.
zygoloid wrote:
I found this comment a bit confusing, because `move` produces a reference
rather than a temporary. How about something like "Pass xvalues to llvm::concat
to request that it makes a copy."
https://github.com/llvm/llvm-project/pull/121417
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][P1061] Add stuctured binding packs (PR #121417)
@@ -951,28 +959,124 @@ Sema::ActOnDecompositionDeclarator(Scope *S, Declarator
&D,
return New;
}
+// CheckBindingsCount
+// - Checks the arity of the structured bindings
+// - Creates the resolved pack expr if there is
+//one
+static bool CheckBindingsCount(Sema &S, DecompositionDecl *DD,
+ QualType DecompType,
+ ArrayRef Bindings,
+ unsigned MemberCount) {
+ auto BindingWithPackItr =
+ std::find_if(Bindings.begin(), Bindings.end(),
+ [](BindingDecl *D) -> bool { return D->isParameterPack();
});
+ bool HasPack = BindingWithPackItr != Bindings.end();
+ bool IsValid;
+ if (!HasPack) {
+IsValid = Bindings.size() == MemberCount;
+ } else {
+// there may not be more members than non-pack bindings
+IsValid = MemberCount >= Bindings.size() - 1;
+ }
+
+ if (IsValid && HasPack) {
+// create the pack expr and assign it to the binding
+unsigned PackSize = MemberCount - Bindings.size() + 1;
+QualType PackType = S.Context.getPackExpansionType(
+S.Context.DependentTy, std::nullopt, /*ExpectsPackInType=*/false);
+BindingDecl *BD = (*BindingWithPackItr);
+BD->setBinding(PackType,
+ ResolvedUnexpandedPackExpr::Create(
+ S.Context, DD->getBeginLoc(), DecompType, PackSize));
zygoloid wrote:
Yes, I prefer that too.
https://github.com/llvm/llvm-project/pull/121417
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][P1061] Add stuctured binding packs (PR #121417)
@@ -951,28 +959,124 @@ Sema::ActOnDecompositionDeclarator(Scope *S, Declarator &D, return New; } +// CheckBindingsCount zygoloid wrote: We [don't start function comments with the function name](https://llvm.org/docs/CodingStandards.html#commenting:~:text=Don%E2%80%99t%20duplicate%20function%20or%20class%20name%20at%20the%20beginning%20of%20the%20comment.). (We used to but that's an old style we're phasing out.) https://github.com/llvm/llvm-project/pull/121417 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][P1061] Add stuctured binding packs (PR #121417)
cor3ntin wrote: > I do not have a strong preference, but I kept `bindings()` the same since it > represents the structure of the AST and that is how it is used in most > places. (like 2x time more than `flat_bindings()`) Lets not then I'm inclined to say we should merge that once @zygoloid and @zwuis are happy their concerns are addressed (+ more tests https://github.com/llvm/llvm-project/pull/121417/files#r1921058349) We also need - A release note (clang/docs/ReleaseNotes.rst) - to update clang/www/cxx_status.html We should hold off updating the feature test macro though. WDYT? https://github.com/llvm/llvm-project/pull/121417 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][P1061] Add stuctured binding packs (PR #121417)
ricejasonf wrote: > I wonder if we should rename flat_bindings() to bindings() and bindings() to > bindings_as_written() (or similar), as I expect that to be the function we > ~always want to use. I do not have a strong preference, but I kept `bindings()` the same since it represents the structure of the AST and that is how it is used in most places. (like 2x time more than `flat_bindings()`) https://github.com/llvm/llvm-project/pull/121417 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][P1061] Add stuctured binding packs (PR #121417)
@@ -5321,6 +5321,59 @@ class BuiltinBitCastExpr final
}
};
+// Represents an unexpanded pack where the list of expressions are
+// known. These are used when structured bindings introduce a pack.
+class ResolvedUnexpandedPackExpr final
+: public Expr,
+ private llvm::TrailingObjects {
ricejasonf wrote:
Personally, I am content to leave it as is, but storing Decls instead of Exprs
does have the benefit of delaying building the DeclRefExprs until expansion
which creates less garbage and would simplify `flat_bindings`. If anything, I
would leave the name `ResolvedUnexpandedPackExpr` and make the trailing objects
`ValueDecl` to support future consolidation. That is premised on never wanting
to create a pack of expressions which was my original intent as this was made
for P1221 (which I have abandoned). I will defer to @cor3ntin.
https://github.com/llvm/llvm-project/pull/121417
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][P1061] Add stuctured binding packs (PR #121417)
@@ -5321,6 +5321,59 @@ class BuiltinBitCastExpr final
}
};
+// Represents an unexpanded pack where the list of expressions are
+// known. These are used when structured bindings introduce a pack.
+class ResolvedUnexpandedPackExpr final
+: public Expr,
+ private llvm::TrailingObjects {
zwuis wrote:
> ... `BindingPackExpr`?
This is what I think: using `BindingPackExpr` and modifying it when
implementing other features (member packs etc).
https://github.com/llvm/llvm-project/pull/121417
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][P1061] Add stuctured binding packs (PR #121417)
@@ -5321,6 +5321,59 @@ class BuiltinBitCastExpr final
}
};
+// Represents an unexpanded pack where the list of expressions are
+// known. These are used when structured bindings introduce a pack.
+class ResolvedUnexpandedPackExpr final
+: public Expr,
+ private llvm::TrailingObjects {
ricejasonf wrote:
Probably. That would simplify the DeclRefExpr stuff and a few other things.
`BindingPackExpr`?
https://github.com/llvm/llvm-project/pull/121417
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][P1061] Add stuctured binding packs (PR #121417)
@@ -5321,6 +5321,59 @@ class BuiltinBitCastExpr final
}
};
+// Represents an unexpanded pack where the list of expressions are
+// known. These are used when structured bindings introduce a pack.
+class ResolvedUnexpandedPackExpr final
+: public Expr,
+ private llvm::TrailingObjects {
zwuis wrote:
Can we store `BindingDecl *` in trailing objects like `FunctionParmPackExpr`?
https://github.com/llvm/llvm-project/pull/121417
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][P1061] Add stuctured binding packs (PR #121417)
@@ -951,28 +959,124 @@ Sema::ActOnDecompositionDeclarator(Scope *S, Declarator
&D,
return New;
}
+// CheckBindingsCount
+// - Checks the arity of the structured bindings
+// - Creates the resolved pack expr if there is
+//one
+static bool CheckBindingsCount(Sema &S, DecompositionDecl *DD,
+ QualType DecompType,
+ ArrayRef Bindings,
+ unsigned MemberCount) {
+ auto BindingWithPackItr =
+ std::find_if(Bindings.begin(), Bindings.end(),
+ [](BindingDecl *D) -> bool { return D->isParameterPack();
});
+ bool HasPack = BindingWithPackItr != Bindings.end();
+ bool IsValid;
+ if (!HasPack) {
+IsValid = Bindings.size() == MemberCount;
+ } else {
+// there may not be more members than non-pack bindings
+IsValid = MemberCount >= Bindings.size() - 1;
+ }
+
+ if (IsValid && HasPack) {
+// create the pack expr and assign it to the binding
+unsigned PackSize = MemberCount - Bindings.size() + 1;
+QualType PackType = S.Context.getPackExpansionType(
+S.Context.DependentTy, std::nullopt, /*ExpectsPackInType=*/false);
+BindingDecl *BD = (*BindingWithPackItr);
+BD->setBinding(PackType,
+ ResolvedUnexpandedPackExpr::Create(
+ S.Context, DD->getBeginLoc(), DecompType, PackSize));
cor3ntin wrote:
I think I like that much better, yes. Thanks!
https://github.com/llvm/llvm-project/pull/121417
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][P1061] Add stuctured binding packs (PR #121417)
@@ -951,28 +959,124 @@ Sema::ActOnDecompositionDeclarator(Scope *S, Declarator
&D,
return New;
}
+// CheckBindingsCount
+// - Checks the arity of the structured bindings
+// - Creates the resolved pack expr if there is
+//one
+static bool CheckBindingsCount(Sema &S, DecompositionDecl *DD,
+ QualType DecompType,
+ ArrayRef Bindings,
+ unsigned MemberCount) {
+ auto BindingWithPackItr =
+ std::find_if(Bindings.begin(), Bindings.end(),
+ [](BindingDecl *D) -> bool { return D->isParameterPack();
});
+ bool HasPack = BindingWithPackItr != Bindings.end();
+ bool IsValid;
+ if (!HasPack) {
+IsValid = Bindings.size() == MemberCount;
+ } else {
+// there may not be more members than non-pack bindings
+IsValid = MemberCount >= Bindings.size() - 1;
+ }
+
+ if (IsValid && HasPack) {
+// create the pack expr and assign it to the binding
+unsigned PackSize = MemberCount - Bindings.size() + 1;
+QualType PackType = S.Context.getPackExpansionType(
+S.Context.DependentTy, std::nullopt, /*ExpectsPackInType=*/false);
+BindingDecl *BD = (*BindingWithPackItr);
+BD->setBinding(PackType,
+ ResolvedUnexpandedPackExpr::Create(
+ S.Context, DD->getBeginLoc(), DecompType, PackSize));
ricejasonf wrote:
Here is a PR to this branch so you can see what would have to change to remove
BindingInitWalker like that. Let me know, and I will add the commit to this
branch.
https://github.com/ricejasonf/llvm-project/pull/1/files
https://github.com/llvm/llvm-project/pull/121417
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][P1061] Add stuctured binding packs (PR #121417)
@@ -951,28 +959,124 @@ Sema::ActOnDecompositionDeclarator(Scope *S, Declarator
&D,
return New;
}
+// CheckBindingsCount
+// - Checks the arity of the structured bindings
+// - Creates the resolved pack expr if there is
+//one
+static bool CheckBindingsCount(Sema &S, DecompositionDecl *DD,
+ QualType DecompType,
+ ArrayRef Bindings,
+ unsigned MemberCount) {
+ auto BindingWithPackItr =
+ std::find_if(Bindings.begin(), Bindings.end(),
+ [](BindingDecl *D) -> bool { return D->isParameterPack();
});
+ bool HasPack = BindingWithPackItr != Bindings.end();
+ bool IsValid;
+ if (!HasPack) {
+IsValid = Bindings.size() == MemberCount;
+ } else {
+// there may not be more members than non-pack bindings
+IsValid = MemberCount >= Bindings.size() - 1;
+ }
+
+ if (IsValid && HasPack) {
+// create the pack expr and assign it to the binding
+unsigned PackSize = MemberCount - Bindings.size() + 1;
+QualType PackType = S.Context.getPackExpansionType(
+S.Context.DependentTy, std::nullopt, /*ExpectsPackInType=*/false);
+BindingDecl *BD = (*BindingWithPackItr);
+BD->setBinding(PackType,
+ ResolvedUnexpandedPackExpr::Create(
+ S.Context, DD->getBeginLoc(), DecompType, PackSize));
ricejasonf wrote:
Oh yeah, using `flat_bindings` there looks doable if we put the
ResolvedUnexpandedPack in there ahead of it like that.
https://github.com/llvm/llvm-project/pull/121417
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][P1061] Add stuctured binding packs (PR #121417)
@@ -951,28 +959,124 @@ Sema::ActOnDecompositionDeclarator(Scope *S, Declarator
&D,
return New;
}
+// CheckBindingsCount
+// - Checks the arity of the structured bindings
+// - Creates the resolved pack expr if there is
+//one
+static bool CheckBindingsCount(Sema &S, DecompositionDecl *DD,
+ QualType DecompType,
+ ArrayRef Bindings,
+ unsigned MemberCount) {
+ auto BindingWithPackItr =
+ std::find_if(Bindings.begin(), Bindings.end(),
+ [](BindingDecl *D) -> bool { return D->isParameterPack();
});
+ bool HasPack = BindingWithPackItr != Bindings.end();
+ bool IsValid;
+ if (!HasPack) {
+IsValid = Bindings.size() == MemberCount;
+ } else {
+// there may not be more members than non-pack bindings
+IsValid = MemberCount >= Bindings.size() - 1;
+ }
+
+ if (IsValid && HasPack) {
+// create the pack expr and assign it to the binding
+unsigned PackSize = MemberCount - Bindings.size() + 1;
+QualType PackType = S.Context.getPackExpansionType(
+S.Context.DependentTy, std::nullopt, /*ExpectsPackInType=*/false);
+BindingDecl *BD = (*BindingWithPackItr);
+BD->setBinding(PackType,
+ ResolvedUnexpandedPackExpr::Create(
+ S.Context, DD->getBeginLoc(), DecompType, PackSize));
cor3ntin wrote:
can we do at least do something like
```cpp
auto* DD = cast(Src);
if (CheckBindingsCount(S, DD, DecompType, Bindings,
NumElems))
return true;
if(ResolvePackInDecomposition(DD, /*TotalSize=*/NumElems)) // create the
nested bindings but does not call `setBindings`
return true;
for (auto *BD : DD->flat_bindings()) {
/*...*/
BD->setBinding( /*...*/)
}
```
IE, my question is can we reuse `flat_bindings()` and not have
`BindingInitWalker`
https://github.com/llvm/llvm-project/pull/121417
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][P1061] Add stuctured binding packs (PR #121417)
https://github.com/cor3ntin edited https://github.com/llvm/llvm-project/pull/121417 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][P1061] Add stuctured binding packs (PR #121417)
@@ -1099,6 +1099,13 @@ def err_lambda_capture_misplaced_ellipsis : Error< "the name of the capture">; def err_lambda_capture_multiple_ellipses : Error< "multiple ellipses in pack capture">; +def err_binding_multiple_ellipses : Error< + "multiple packs in structured binding declaration">; +def note_previous_ellipsis : Note< + "previous binding pack specified here">; +def ext_cxx_binding_pack : ExtWarn< cor3ntin wrote: The usual pattern is that In older language modes we say "xxx is a c++YY extension" and in c++YY we say "xxx is incompatible with C++ standards before C++YY" (and that warning is in the CXXPreYYCompat group and DefaultIgnore) I should have told you to search for `CXXPre26Compat` for examples :) https://github.com/llvm/llvm-project/pull/121417 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][P1061] Add stuctured binding packs (PR #121417)
@@ -1099,6 +1099,13 @@ def err_lambda_capture_misplaced_ellipsis : Error< "the name of the capture">; def err_lambda_capture_multiple_ellipses : Error< "multiple ellipses in pack capture">; +def err_binding_multiple_ellipses : Error< + "multiple packs in structured binding declaration">; +def note_previous_ellipsis : Note< + "previous binding pack specified here">; +def ext_cxx_binding_pack : ExtWarn< ricejasonf wrote: What warning are we wanting to display in C++26 mode? The `ext_cxx_binding_pack` was added for when we are **not** in C++26 mode right? https://github.com/llvm/llvm-project/pull/121417 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][P1061] Add stuctured binding packs (PR #121417)
@@ -951,28 +959,124 @@ Sema::ActOnDecompositionDeclarator(Scope *S, Declarator
&D,
return New;
}
+// CheckBindingsCount
+// - Checks the arity of the structured bindings
+// - Creates the resolved pack expr if there is
+//one
+static bool CheckBindingsCount(Sema &S, DecompositionDecl *DD,
+ QualType DecompType,
+ ArrayRef Bindings,
+ unsigned MemberCount) {
+ auto BindingWithPackItr =
+ std::find_if(Bindings.begin(), Bindings.end(),
+ [](BindingDecl *D) -> bool { return D->isParameterPack();
});
+ bool HasPack = BindingWithPackItr != Bindings.end();
+ bool IsValid;
+ if (!HasPack) {
+IsValid = Bindings.size() == MemberCount;
+ } else {
+// there may not be more members than non-pack bindings
+IsValid = MemberCount >= Bindings.size() - 1;
+ }
+
+ if (IsValid && HasPack) {
+// create the pack expr and assign it to the binding
+unsigned PackSize = MemberCount - Bindings.size() + 1;
+QualType PackType = S.Context.getPackExpansionType(
+S.Context.DependentTy, std::nullopt, /*ExpectsPackInType=*/false);
+BindingDecl *BD = (*BindingWithPackItr);
+BD->setBinding(PackType,
+ ResolvedUnexpandedPackExpr::Create(
+ S.Context, DD->getBeginLoc(), DecompType, PackSize));
Sirraide wrote:
> What is IF? (initializer first?)
‘ill-formed’, presumably
https://github.com/llvm/llvm-project/pull/121417
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][P1061] Add stuctured binding packs (PR #121417)
@@ -951,28 +959,124 @@ Sema::ActOnDecompositionDeclarator(Scope *S, Declarator
&D,
return New;
}
+// CheckBindingsCount
+// - Checks the arity of the structured bindings
+// - Creates the resolved pack expr if there is
+//one
+static bool CheckBindingsCount(Sema &S, DecompositionDecl *DD,
+ QualType DecompType,
+ ArrayRef Bindings,
+ unsigned MemberCount) {
+ auto BindingWithPackItr =
+ std::find_if(Bindings.begin(), Bindings.end(),
+ [](BindingDecl *D) -> bool { return D->isParameterPack();
});
+ bool HasPack = BindingWithPackItr != Bindings.end();
+ bool IsValid;
+ if (!HasPack) {
+IsValid = Bindings.size() == MemberCount;
+ } else {
+// there may not be more members than non-pack bindings
+IsValid = MemberCount >= Bindings.size() - 1;
+ }
+
+ if (IsValid && HasPack) {
+// create the pack expr and assign it to the binding
+unsigned PackSize = MemberCount - Bindings.size() + 1;
+QualType PackType = S.Context.getPackExpansionType(
+S.Context.DependentTy, std::nullopt, /*ExpectsPackInType=*/false);
+BindingDecl *BD = (*BindingWithPackItr);
+BD->setBinding(PackType,
+ ResolvedUnexpandedPackExpr::Create(
+ S.Context, DD->getBeginLoc(), DecompType, PackSize));
ricejasonf wrote:
What is IF? (initializer first?)
I have gone down this rabbit hole a couple of times. The initializer for a
variable is built differently based on the context. For instance, range based
for loops defer building the initializer (for whatever reason idk,) but it all
happens in the call to VisitVarDecl which builds the new DecompositionDecl. I
do not know what it would take to rip all of that out of there.
See
https://github.com/ricejasonf/llvm-project/blob/ricejasonf/p1061r9/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp#L1193
and
https://github.com/ricejasonf/llvm-project/blob/ricejasonf/p1061r9/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp#L1224
The above attempt was building the initializer just to get the number of
bindings and discarding it which felt like a waste.
Forgive me, IF I am misunderstanding what you are saying here. :rofl:
https://github.com/llvm/llvm-project/pull/121417
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][P1061] Add stuctured binding packs (PR #121417)
@@ -1099,6 +1099,13 @@ def err_lambda_capture_misplaced_ellipsis : Error< "the name of the capture">; def err_lambda_capture_multiple_ellipses : Error< "multiple ellipses in pack capture">; +def err_binding_multiple_ellipses : Error< + "multiple packs in structured binding declaration">; +def note_previous_ellipsis : Note< + "previous binding pack specified here">; +def ext_cxx_binding_pack : ExtWarn< cor3ntin wrote: We need a corresponding warning to display in c++26 mode for example ```cpp Diag(LambdaBeginLoc, getLangOpts().CPlusPlus11 ? diag::warn_cxx98_compat_lambda : diag::ext_lambda) ``` (look at other `ext_` diags for more example) https://github.com/llvm/llvm-project/pull/121417 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][P1061] Add stuctured binding packs (PR #121417)
https://github.com/cor3ntin edited https://github.com/llvm/llvm-project/pull/121417 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][P1061] Add stuctured binding packs (PR #121417)
@@ -0,0 +1,117 @@ +// RUN: %clang_cc1 -fsyntax-only -std=c++26 %s -verify cor3ntin wrote: Can we - Test that `sizeof` (in the case where it's empty, of size 1, n, etc_) works fine - Test that `auto [a, b...]` works for an array of size 1 - Test that `auto [a, b..., c]` errors for an array of size 1 - Test that it works fine with pack indexing https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2024/p1061r10.html#pnum_5 Adding that verbatim would be a way to write some of these Can we add this example? https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2024/p1061r10.html#pnum_6 Can we add this example https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2024/p1061r10.html#pnum_22 https://github.com/llvm/llvm-project/pull/121417 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][P1061] Add stuctured binding packs (PR #121417)
https://github.com/cor3ntin commented: A few more comments on top of Richard's https://github.com/llvm/llvm-project/pull/121417 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][P1061] Add stuctured binding packs (PR #121417)
@@ -951,28 +959,124 @@ Sema::ActOnDecompositionDeclarator(Scope *S, Declarator
&D,
return New;
}
+// CheckBindingsCount
+// - Checks the arity of the structured bindings
+// - Creates the resolved pack expr if there is
+//one
+static bool CheckBindingsCount(Sema &S, DecompositionDecl *DD,
+ QualType DecompType,
+ ArrayRef Bindings,
+ unsigned MemberCount) {
+ auto BindingWithPackItr =
+ std::find_if(Bindings.begin(), Bindings.end(),
+ [](BindingDecl *D) -> bool { return D->isParameterPack();
});
+ bool HasPack = BindingWithPackItr != Bindings.end();
+ bool IsValid;
+ if (!HasPack) {
+IsValid = Bindings.size() == MemberCount;
+ } else {
+// there may not be more members than non-pack bindings
+IsValid = MemberCount >= Bindings.size() - 1;
+ }
+
+ if (IsValid && HasPack) {
+// create the pack expr and assign it to the binding
+unsigned PackSize = MemberCount - Bindings.size() + 1;
+QualType PackType = S.Context.getPackExpansionType(
+S.Context.DependentTy, std::nullopt, /*ExpectsPackInType=*/false);
+BindingDecl *BD = (*BindingWithPackItr);
+BD->setBinding(PackType,
+ ResolvedUnexpandedPackExpr::Create(
+ S.Context, DD->getBeginLoc(), DecompType, PackSize));
cor3ntin wrote:
Hum. Did you try to transform the initializer first?
I don't think there is anything stopping us from trying as the following is IF
```
struct S { int i; };
auto [a] = S{a};
```
https://github.com/llvm/llvm-project/pull/121417
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][P1061] Add stuctured binding packs (PR #121417)
@@ -757,23 +775,40 @@ bool Sema::CheckParameterPacksForExpansion(
bool HaveFirstPack = false;
std::optional NumPartialExpansions;
SourceLocation PartiallySubstitutedPackLoc;
+ typedef LocalInstantiationScope::DeclArgumentPack DeclArgumentPack;
for (UnexpandedParameterPack ParmPack : Unexpanded) {
// Compute the depth and index for this parameter pack.
unsigned Depth = 0, Index = 0;
IdentifierInfo *Name;
bool IsVarDeclPack = false;
+ResolvedUnexpandedPackExpr *ResolvedPack = nullptr;
if (const TemplateTypeParmType *TTP =
ParmPack.first.dyn_cast()) {
Depth = TTP->getDepth();
Index = TTP->getIndex();
Name = TTP->getIdentifier();
+} else if (auto *RP =
+ ParmPack.first.dyn_cast()) {
+ ResolvedPack = RP;
} else {
NamedDecl *ND = cast(ParmPack.first);
if (isa(ND))
IsVarDeclPack = true;
- else
+ else if (isa(ND)) {
+// Find the instantiated BindingDecl and check it for a resolved pack.
+llvm::PointerUnion *Instantiation =
+CurrentInstantiationScope->findInstantiationOf(ND);
+Decl *B = cast(*Instantiation);
+Expr *BindingExpr = cast(B)->getBinding();
+ResolvedPack =
+dyn_cast_if_present(BindingExpr);
+if (!ResolvedPack) {
+ ShouldExpand = false;
+ continue;
+}
zyn0217 wrote:
Oh yes indeed. I missed something in CheckCompleteDecompositionDeclaration() -
we wouldn't create ResolvedUnexpandedPackExpr for dependent types.
https://github.com/llvm/llvm-project/pull/121417
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][P1061] Add stuctured binding packs (PR #121417)
@@ -15991,6 +15998,24 @@
TreeTransform::TransformFunctionParmPackExpr(FunctionParmPackExpr *E) {
return E;
}
+template
+ExprResult TreeTransform::TransformResolvedUnexpandedPackExpr(
+ResolvedUnexpandedPackExpr *E) {
+ bool ArgumentChanged = false;
+ SmallVector NewExprs;
+ if (TransformExprs(E->getExprs().begin(), E->getNumExprs(),
+ /*IsCall=*/false, NewExprs, &ArgumentChanged))
+return ExprError();
+
+ if (!AlwaysRebuild() && !ArgumentChanged)
+return E;
+
+ // NOTE: The type is just a superficial PackExpansionType
+ // that needs no substitution.
zyn0217 wrote:
NVM ResolvedUnexpandedPackExpr is not supposed to be present in the eventual
instantiation - it's like FunctionParmPackExpr and I was just momentarily
confused by the name "Resolved".
https://github.com/llvm/llvm-project/pull/121417
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][P1061] Add stuctured binding packs (PR #121417)
@@ -15991,6 +15998,24 @@
TreeTransform::TransformFunctionParmPackExpr(FunctionParmPackExpr *E) {
return E;
}
+template
+ExprResult TreeTransform::TransformResolvedUnexpandedPackExpr(
+ResolvedUnexpandedPackExpr *E) {
+ bool ArgumentChanged = false;
+ SmallVector NewExprs;
+ if (TransformExprs(E->getExprs().begin(), E->getNumExprs(),
+ /*IsCall=*/false, NewExprs, &ArgumentChanged))
+return ExprError();
+
+ if (!AlwaysRebuild() && !ArgumentChanged)
+return E;
+
+ // NOTE: The type is just a superficial PackExpansionType
+ // that needs no substitution.
zyn0217 wrote:
I was actually thinking, would we end up with a dependent PackExpansionType on
ResolvedUnexpandedPackExpr in the eventual instantiation? Like, in an AST dump,
do we see a on the instantiated node?
https://github.com/llvm/llvm-project/pull/121417
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][P1061] Add stuctured binding packs (PR #121417)
https://github.com/ricejasonf edited https://github.com/llvm/llvm-project/pull/121417 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][P1061] Add stuctured binding packs (PR #121417)
https://github.com/ricejasonf edited https://github.com/llvm/llvm-project/pull/121417 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][P1061] Add stuctured binding packs (PR #121417)
https://github.com/ricejasonf edited https://github.com/llvm/llvm-project/pull/121417 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][P1061] Add stuctured binding packs (PR #121417)
@@ -951,28 +959,124 @@ Sema::ActOnDecompositionDeclarator(Scope *S, Declarator
&D,
return New;
}
+// CheckBindingsCount
+// - Checks the arity of the structured bindings
+// - Creates the resolved pack expr if there is
+//one
+static bool CheckBindingsCount(Sema &S, DecompositionDecl *DD,
+ QualType DecompType,
+ ArrayRef Bindings,
+ unsigned MemberCount) {
+ auto BindingWithPackItr =
+ std::find_if(Bindings.begin(), Bindings.end(),
+ [](BindingDecl *D) -> bool { return D->isParameterPack();
});
+ bool HasPack = BindingWithPackItr != Bindings.end();
+ bool IsValid;
+ if (!HasPack) {
+IsValid = Bindings.size() == MemberCount;
+ } else {
+// there may not be more members than non-pack bindings
+IsValid = MemberCount >= Bindings.size() - 1;
+ }
+
+ if (IsValid && HasPack) {
+// create the pack expr and assign it to the binding
+unsigned PackSize = MemberCount - Bindings.size() + 1;
+QualType PackType = S.Context.getPackExpansionType(
+S.Context.DependentTy, std::nullopt, /*ExpectsPackInType=*/false);
+BindingDecl *BD = (*BindingWithPackItr);
+BD->setBinding(PackType,
+ ResolvedUnexpandedPackExpr::Create(
+ S.Context, DD->getBeginLoc(), DecompType, PackSize));
ricejasonf wrote:
>(During the initial parse, we should treat a structured binding declaration
>with a binding pack as being dependent, just like we treat a case where the
>initializer has an unexpanded pack as dependent.)
The BindingDecls are still created even when the initializer is dependent, and
still the init expression is created after the DecompositionDecl even in
template instantiation. (I remember trying this, but template instantation does
not like instantiation of the same local decl twice.)
https://github.com/llvm/llvm-project/pull/121417
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][P1061] Add stuctured binding packs (PR #121417)
@@ -951,28 +959,124 @@ Sema::ActOnDecompositionDeclarator(Scope *S, Declarator
&D,
return New;
}
+// CheckBindingsCount
+// - Checks the arity of the structured bindings
+// - Creates the resolved pack expr if there is
+//one
+static bool CheckBindingsCount(Sema &S, DecompositionDecl *DD,
+ QualType DecompType,
+ ArrayRef Bindings,
+ unsigned MemberCount) {
+ auto BindingWithPackItr =
+ std::find_if(Bindings.begin(), Bindings.end(),
+ [](BindingDecl *D) -> bool { return D->isParameterPack();
});
+ bool HasPack = BindingWithPackItr != Bindings.end();
+ bool IsValid;
+ if (!HasPack) {
+IsValid = Bindings.size() == MemberCount;
+ } else {
+// there may not be more members than non-pack bindings
+IsValid = MemberCount >= Bindings.size() - 1;
+ }
+
+ if (IsValid && HasPack) {
+// create the pack expr and assign it to the binding
+unsigned PackSize = MemberCount - Bindings.size() + 1;
+QualType PackType = S.Context.getPackExpansionType(
+S.Context.DependentTy, std::nullopt, /*ExpectsPackInType=*/false);
+BindingDecl *BD = (*BindingWithPackItr);
+BD->setBinding(PackType,
+ ResolvedUnexpandedPackExpr::Create(
+ S.Context, DD->getBeginLoc(), DecompType, PackSize));
ricejasonf wrote:
The problem is that we do not know the length of the pack until the initializer
expression is created. This happens in various locations depending on context
such as range based for loops. I am not saying it isn't possible, but I think
it would require creating the init expression up front which I do not know if
that would be problematic with how variables are checked. We could also do
Bindings in a separate allocation, but that would require keeping the
DecompositionDeclarator around somehow, and there would need to be that extra
information in the DecompositionDecl which would have a cost for Decomps that
do not have a pack. (I don't know if that is an issue.)
https://github.com/llvm/llvm-project/pull/121417
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][P1061] Add stuctured binding packs (PR #121417)
@@ -5321,6 +5321,59 @@ class BuiltinBitCastExpr final } }; +// Represents an unexpanded pack where the list of expressions are +// known. These are used when structured bindings introduce a pack. +class ResolvedUnexpandedPackExpr final ricejasonf wrote: I definitely think they could be generalized which is what I had in mind with `ResolvedUnexpandedPackExpr`. This is originally what I had for parmexprs (P1221). https://github.com/llvm/llvm-project/pull/121417 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][P1061] Add stuctured binding packs (PR #121417)
@@ -951,28 +959,124 @@ Sema::ActOnDecompositionDeclarator(Scope *S, Declarator
&D,
return New;
}
+// CheckBindingsCount
+// - Checks the arity of the structured bindings
+// - Creates the resolved pack expr if there is
+//one
+static bool CheckBindingsCount(Sema &S, DecompositionDecl *DD,
+ QualType DecompType,
+ ArrayRef Bindings,
+ unsigned MemberCount) {
+ auto BindingWithPackItr =
+ std::find_if(Bindings.begin(), Bindings.end(),
+ [](BindingDecl *D) -> bool { return D->isParameterPack();
});
+ bool HasPack = BindingWithPackItr != Bindings.end();
+ bool IsValid;
+ if (!HasPack) {
+IsValid = Bindings.size() == MemberCount;
+ } else {
+// there may not be more members than non-pack bindings
+IsValid = MemberCount >= Bindings.size() - 1;
+ }
+
+ if (IsValid && HasPack) {
+// create the pack expr and assign it to the binding
+unsigned PackSize = MemberCount - Bindings.size() + 1;
+QualType PackType = S.Context.getPackExpansionType(
+S.Context.DependentTy, std::nullopt, /*ExpectsPackInType=*/false);
+BindingDecl *BD = (*BindingWithPackItr);
+BD->setBinding(PackType,
+ ResolvedUnexpandedPackExpr::Create(
+ S.Context, DD->getBeginLoc(), DecompType, PackSize));
zygoloid wrote:
Instead of creating a single binding with a pack of values, I think template
instantiation should be creating `N` `BindingDecl`s each with a single value,
like we do when we instantiate a function parameter pack or a template
parameter pack. I think that would simplify a lot of what you're doing here.
(During the initial parse, we should treat a structured binding declaration
with a binding pack as being dependent, just like we treat a case where the
initializer has an unexpanded pack as dependent.)
https://github.com/llvm/llvm-project/pull/121417
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][P1061] Add stuctured binding packs (PR #121417)
@@ -1099,6 +1099,13 @@ def err_lambda_capture_misplaced_ellipsis : Error< "the name of the capture">; def err_lambda_capture_multiple_ellipses : Error< "multiple ellipses in pack capture">; +def err_binding_multiple_ellipses : Error< + "multiple ellipses in structured binding declaration">; +def note_previous_ellipsis : Note< + "previous ellipsis specified here">; zygoloid wrote: It seems a little strange to talk about ellipses rather than packs here. Maybe "multiple binding packs in [...]" instead? https://github.com/llvm/llvm-project/pull/121417 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][P1061] Add stuctured binding packs (PR #121417)
@@ -5321,6 +5321,59 @@ class BuiltinBitCastExpr final } }; +// Represents an unexpanded pack where the list of expressions are +// known. These are used when structured bindings introduce a pack. +class ResolvedUnexpandedPackExpr final zygoloid wrote: This seems to be doing the same job as `SubstNonTypeTemplateParmPackExpr` and `FunctionParmPackExpr`. Do we need all three? If so, mentioning the relationship between the three in this comment might be nice -- but I do wonder if `FunctionParmPackExpr` could be generalized to cover the case of a binding declaration too. https://github.com/llvm/llvm-project/pull/121417 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][P1061] Add stuctured binding packs (PR #121417)
github-actions[bot] wrote:
:warning: C/C++ code formatter, clang-format found issues in your code.
:warning:
You can test this locally with the following command:
``bash
git-clang-format --diff 6e8a1a45a783c13e4cd19bfd20b7a56cab6f7d81
e0cb7487cec4d9eb3aac4806d99cea2120f6974a --extensions h,cpp --
clang/test/AST/ast-dump-binding-pack.cpp
clang/test/CodeGenCXX/cxx2c-decomposition.cpp
clang/test/Parser/cxx2c-binding-pack.cpp
clang/test/SemaCXX/cxx2c-binding-pack-nontemplate.cpp
clang/test/SemaCXX/cxx2c-binding-pack.cpp clang/include/clang/AST/Decl.h
clang/include/clang/AST/DeclCXX.h clang/include/clang/AST/ExprCXX.h
clang/include/clang/AST/RecursiveASTVisitor.h
clang/include/clang/Sema/DeclSpec.h clang/include/clang/Sema/Sema.h
clang/include/clang/Serialization/ASTBitCodes.h clang/lib/AST/ASTContext.cpp
clang/lib/AST/ASTImporter.cpp clang/lib/AST/Decl.cpp clang/lib/AST/DeclBase.cpp
clang/lib/AST/DeclCXX.cpp clang/lib/AST/Expr.cpp clang/lib/AST/ExprCXX.cpp
clang/lib/AST/ExprClassification.cpp clang/lib/AST/ExprConstant.cpp
clang/lib/AST/ItaniumMangle.cpp clang/lib/AST/StmtPrinter.cpp
clang/lib/AST/StmtProfile.cpp clang/lib/CodeGen/CGDebugInfo.cpp
clang/lib/CodeGen/CGDecl.cpp clang/lib/CodeGen/CodeGenModule.cpp
clang/lib/Parse/ParseDecl.cpp clang/lib/Sema/SemaDeclCXX.cpp
clang/lib/Sema/SemaExceptionSpec.cpp clang/lib/Sema/SemaStmt.cpp
clang/lib/Sema/SemaTemplateInstantiate.cpp
clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
clang/lib/Sema/SemaTemplateVariadic.cpp clang/lib/Sema/TreeTransform.h
clang/lib/Serialization/ASTReaderStmt.cpp clang/lib/Serialization/ASTWriter.cpp
clang/lib/Serialization/ASTWriterStmt.cpp
clang/lib/StaticAnalyzer/Core/ExprEngine.cpp clang/tools/libclang/CXCursor.cpp
``
View the diff from clang-format here.
``diff
diff --git a/clang/lib/Sema/SemaTemplateVariadic.cpp
b/clang/lib/Sema/SemaTemplateVariadic.cpp
index 4d6499d8e8..3c56794722 100644
--- a/clang/lib/Sema/SemaTemplateVariadic.cpp
+++ b/clang/lib/Sema/SemaTemplateVariadic.cpp
@@ -802,8 +802,7 @@ bool Sema::CheckParameterPacksForExpansion(
CurrentInstantiationScope->findInstantiationOf(ND);
Decl *B = cast(*Instantiation);
Expr *BindingExpr = cast(B)->getBinding();
-ResolvedPack =
-cast_if_present(BindingExpr);
+ResolvedPack =
cast_if_present(BindingExpr);
if (!ResolvedPack) {
ShouldExpand = false;
continue;
``
https://github.com/llvm/llvm-project/pull/121417
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][P1061] Add stuctured binding packs (PR #121417)
@@ -757,23 +775,40 @@ bool Sema::CheckParameterPacksForExpansion(
bool HaveFirstPack = false;
std::optional NumPartialExpansions;
SourceLocation PartiallySubstitutedPackLoc;
+ typedef LocalInstantiationScope::DeclArgumentPack DeclArgumentPack;
for (UnexpandedParameterPack ParmPack : Unexpanded) {
// Compute the depth and index for this parameter pack.
unsigned Depth = 0, Index = 0;
IdentifierInfo *Name;
bool IsVarDeclPack = false;
+ResolvedUnexpandedPackExpr *ResolvedPack = nullptr;
if (const TemplateTypeParmType *TTP =
ParmPack.first.dyn_cast()) {
Depth = TTP->getDepth();
Index = TTP->getIndex();
Name = TTP->getIdentifier();
+} else if (auto *RP =
+ ParmPack.first.dyn_cast()) {
+ ResolvedPack = RP;
} else {
NamedDecl *ND = cast(ParmPack.first);
if (isa(ND))
IsVarDeclPack = true;
- else
+ else if (isa(ND)) {
+// Find the instantiated BindingDecl and check it for a resolved pack.
+llvm::PointerUnion *Instantiation =
+CurrentInstantiationScope->findInstantiationOf(ND);
+Decl *B = cast(*Instantiation);
+Expr *BindingExpr = cast(B)->getBinding();
+ResolvedPack =
+dyn_cast_if_present(BindingExpr);
+if (!ResolvedPack) {
+ ShouldExpand = false;
+ continue;
+}
ricejasonf wrote:
This case is definitely reached. (I tried it.) I think I could make this
`cast_if_present`.
https://github.com/llvm/llvm-project/pull/121417
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][P1061] Add stuctured binding packs (PR #121417)
@@ -15991,6 +15998,24 @@
TreeTransform::TransformFunctionParmPackExpr(FunctionParmPackExpr *E) {
return E;
}
+template
+ExprResult TreeTransform::TransformResolvedUnexpandedPackExpr(
+ResolvedUnexpandedPackExpr *E) {
+ bool ArgumentChanged = false;
+ SmallVector NewExprs;
+ if (TransformExprs(E->getExprs().begin(), E->getNumExprs(),
+ /*IsCall=*/false, NewExprs, &ArgumentChanged))
+return ExprError();
+
+ if (!AlwaysRebuild() && !ArgumentChanged)
+return E;
+
+ // NOTE: The type is just a superficial PackExpansionType
+ // that needs no substitution.
ricejasonf wrote:
I am not sure I understand what you mean here. It is still an unexpanded pack
that were are just rebuilding.
https://github.com/llvm/llvm-project/pull/121417
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][P1061] Add stuctured binding packs (PR #121417)
@@ -50,17 +50,29 @@ class CollectUnexpandedParameterPacksVisitor
auto *FTD = FD ? FD->getDescribedFunctionTemplate() : nullptr;
if (FTD && FTD->getTemplateParameters()->getDepth() >= DepthLimit)
return;
- } else if (getDepthAndIndex(ND).first >= DepthLimit)
+ } else if (auto *BD = dyn_cast(ND)) {
+Expr *E = BD->getBinding();
+if (auto *RP = dyn_cast_if_present(E)) {
+ addUnexpanded(RP);
+ return;
+}
ricejasonf wrote:
I changed it to `cast_if_present` since a dependent decomposition leaves the
binding expr as nullptr.
https://github.com/llvm/llvm-project/pull/121417
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][P1061] Add stuctured binding packs (PR #121417)
@@ -1166,26 +1166,54 @@
TemplateDeclInstantiator::VisitTypeAliasTemplateDecl(TypeAliasTemplateDecl *D) {
Decl *TemplateDeclInstantiator::VisitBindingDecl(BindingDecl *D) {
auto *NewBD = BindingDecl::Create(SemaRef.Context, Owner, D->getLocation(),
-D->getIdentifier());
+D->getIdentifier(), D->getType());
NewBD->setReferenced(D->isReferenced());
SemaRef.CurrentInstantiationScope->InstantiatedLocal(D, NewBD);
+
return NewBD;
}
Decl *TemplateDeclInstantiator::VisitDecompositionDecl(DecompositionDecl *D) {
// Transform the bindings first.
+ // The transformed DD will have all of the concrete BindingDecls.
SmallVector NewBindings;
- for (auto *OldBD : D->bindings())
+ ResolvedUnexpandedPackExpr *OldResolvedPack = nullptr;
+ for (auto *OldBD : D->bindings()) {
+Expr *BindingExpr = OldBD->getBinding();
+if (auto *RP =
dyn_cast_if_present(BindingExpr))
+ OldResolvedPack = RP;
NewBindings.push_back(cast(VisitBindingDecl(OldBD)));
+ }
ArrayRef NewBindingArray = NewBindings;
- auto *NewDD = cast_or_null(
+ auto *NewDD = cast_if_present(
VisitVarDecl(D, /*InstantiatingVarTemplate=*/false, &NewBindingArray));
if (!NewDD || NewDD->isInvalidDecl())
for (auto *NewBD : NewBindings)
NewBD->setInvalidDecl();
+ if (OldResolvedPack) {
+// Mark the holding vars (if any) in the pack as instantiated since
+// they are created implicitly.
+auto Bindings = NewDD->bindings();
+auto BPack = std::find_if(
+Bindings.begin(), Bindings.end(),
+[](BindingDecl *D) -> bool { return D->isParameterPack(); });
+auto *NewResolvedPack =
+cast((*BPack)->getBinding());
+auto OldExprs = OldResolvedPack->getExprs();
+auto NewExprs = NewResolvedPack->getExprs();
ricejasonf wrote:
added
https://github.com/llvm/llvm-project/pull/121417
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][P1061] Add stuctured binding packs (PR #121417)
@@ -1166,26 +1166,54 @@
TemplateDeclInstantiator::VisitTypeAliasTemplateDecl(TypeAliasTemplateDecl *D) {
Decl *TemplateDeclInstantiator::VisitBindingDecl(BindingDecl *D) {
auto *NewBD = BindingDecl::Create(SemaRef.Context, Owner, D->getLocation(),
-D->getIdentifier());
+D->getIdentifier(), D->getType());
NewBD->setReferenced(D->isReferenced());
SemaRef.CurrentInstantiationScope->InstantiatedLocal(D, NewBD);
+
return NewBD;
}
Decl *TemplateDeclInstantiator::VisitDecompositionDecl(DecompositionDecl *D) {
// Transform the bindings first.
+ // The transformed DD will have all of the concrete BindingDecls.
SmallVector NewBindings;
- for (auto *OldBD : D->bindings())
+ ResolvedUnexpandedPackExpr *OldResolvedPack = nullptr;
+ for (auto *OldBD : D->bindings()) {
+Expr *BindingExpr = OldBD->getBinding();
+if (auto *RP =
dyn_cast_if_present(BindingExpr))
+ OldResolvedPack = RP;
ricejasonf wrote:
The `isa` call would fail on a null type so I just left a
comment.
https://github.com/llvm/llvm-project/pull/121417
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][P1061] Add stuctured binding packs (PR #121417)
@@ -757,23 +775,40 @@ bool Sema::CheckParameterPacksForExpansion(
bool HaveFirstPack = false;
std::optional NumPartialExpansions;
SourceLocation PartiallySubstitutedPackLoc;
+ typedef LocalInstantiationScope::DeclArgumentPack DeclArgumentPack;
for (UnexpandedParameterPack ParmPack : Unexpanded) {
// Compute the depth and index for this parameter pack.
unsigned Depth = 0, Index = 0;
IdentifierInfo *Name;
bool IsVarDeclPack = false;
+ResolvedUnexpandedPackExpr *ResolvedPack = nullptr;
if (const TemplateTypeParmType *TTP =
ParmPack.first.dyn_cast()) {
Depth = TTP->getDepth();
Index = TTP->getIndex();
Name = TTP->getIdentifier();
+} else if (auto *RP =
+ ParmPack.first.dyn_cast()) {
+ ResolvedPack = RP;
} else {
NamedDecl *ND = cast(ParmPack.first);
if (isa(ND))
IsVarDeclPack = true;
- else
+ else if (isa(ND)) {
+// Find the instantiated BindingDecl and check it for a resolved pack.
+llvm::PointerUnion *Instantiation =
+CurrentInstantiationScope->findInstantiationOf(ND);
+Decl *B = cast(*Instantiation);
+Expr *BindingExpr = cast(B)->getBinding();
+ResolvedPack =
+dyn_cast_if_present(BindingExpr);
+if (!ResolvedPack) {
+ ShouldExpand = false;
+ continue;
+}
zyn0217 wrote:
I was somehow confused here.
We seem to never actually build a BindingDecl with expressions other than
ResolvedUnexpandedPackExpr to model an 'unexpanded' BindingDecl. Instead, we
put a PackExpansionType to indicate the unexpanded state. So I wonder if this
is ever exercised?
I might be missing something else here though.
https://github.com/llvm/llvm-project/pull/121417
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][P1061] Add stuctured binding packs (PR #121417)
https://github.com/zyn0217 edited https://github.com/llvm/llvm-project/pull/121417 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][P1061] Add stuctured binding packs (PR #121417)
@@ -1508,23 +1612,52 @@ static bool checkMemberDecomposition(Sema &S,
ArrayRef Bindings,
Qualifiers Q = DecompType.getQualifiers();
if (FD->isMutable())
Q.removeConst();
-B->setBinding(S.BuildQualifiedType(FD->getType(), Loc, Q), E.get());
+Walker.commitAndAdvance(S.BuildQualifiedType(FD->getType(), Loc, Q),
+E.get());
}
- if (I != Bindings.size())
-return DiagnoseBadNumberOfBindings();
-
return false;
}
+unsigned Sema::GetDecompositionElementCount(QualType DecompType) {
+ assert(!DecompType->isDependentType() && "expecting non-dependent type");
+ SourceLocation Loc = SourceLocation(); // FIXME
+ DecompType = DecompType.getNonReferenceType();
+ if (auto *CAT = Context.getAsConstantArrayType(DecompType))
+return CAT->getSize().getLimitedValue(UINT_MAX);
+ if (auto *VT = DecompType->getAs())
+return VT->getNumElements();
+ if (auto *CT = DecompType->getAs())
+return 2;
+ llvm::APSInt TupleSize(32);
+ if (IsTupleLike TL = isTupleLike(*this, Loc, DecompType, TupleSize);
+ TL == IsTupleLike::TupleLike)
+return (unsigned)TupleSize.getLimitedValue(UINT_MAX);
+
+ if (CXXRecordDecl *RD = DecompType->getAsCXXRecordDecl();
+ RD && !RD->isUnion()) {
+CXXCastPath BasePath;
+DeclAccessPair BasePair =
+findDecomposableBaseClass(*this, Loc, RD, BasePath);
+RD = cast_or_null(BasePair.getDecl());
+if (RD)
+ return llvm::count_if(
+ RD->fields(), [](FieldDecl *FD) { return !FD->isUnnamedBitField();
});
+ }
+
+ llvm_unreachable("unknown type for decomposition");
+}
+
void Sema::CheckCompleteDecompositionDeclaration(DecompositionDecl *DD) {
QualType DecompType = DD->getType();
// If the type of the decomposition is dependent, then so is the type of
// each binding.
if (DecompType->isDependentType()) {
-for (auto *B : DD->bindings())
- B->setType(Context.DependentTy);
+for (auto *B : DD->bindings()) {
+ if (B->getType().isNull())
+B->setType(Context.DependentTy);
+}
zyn0217 wrote:
Can we clarify the intent here, e.g. `!isa(B->getType())`?
https://github.com/llvm/llvm-project/pull/121417
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][P1061] Add stuctured binding packs (PR #121417)
@@ -1166,26 +1166,54 @@
TemplateDeclInstantiator::VisitTypeAliasTemplateDecl(TypeAliasTemplateDecl *D) {
Decl *TemplateDeclInstantiator::VisitBindingDecl(BindingDecl *D) {
auto *NewBD = BindingDecl::Create(SemaRef.Context, Owner, D->getLocation(),
-D->getIdentifier());
+D->getIdentifier(), D->getType());
NewBD->setReferenced(D->isReferenced());
SemaRef.CurrentInstantiationScope->InstantiatedLocal(D, NewBD);
+
return NewBD;
}
Decl *TemplateDeclInstantiator::VisitDecompositionDecl(DecompositionDecl *D) {
// Transform the bindings first.
+ // The transformed DD will have all of the concrete BindingDecls.
SmallVector NewBindings;
- for (auto *OldBD : D->bindings())
+ ResolvedUnexpandedPackExpr *OldResolvedPack = nullptr;
+ for (auto *OldBD : D->bindings()) {
+Expr *BindingExpr = OldBD->getBinding();
+if (auto *RP =
dyn_cast_if_present(BindingExpr))
+ OldResolvedPack = RP;
NewBindings.push_back(cast(VisitBindingDecl(OldBD)));
+ }
ArrayRef NewBindingArray = NewBindings;
- auto *NewDD = cast_or_null(
+ auto *NewDD = cast_if_present(
VisitVarDecl(D, /*InstantiatingVarTemplate=*/false, &NewBindingArray));
if (!NewDD || NewDD->isInvalidDecl())
for (auto *NewBD : NewBindings)
NewBD->setInvalidDecl();
+ if (OldResolvedPack) {
+// Mark the holding vars (if any) in the pack as instantiated since
+// they are created implicitly.
+auto Bindings = NewDD->bindings();
+auto BPack = std::find_if(
+Bindings.begin(), Bindings.end(),
+[](BindingDecl *D) -> bool { return D->isParameterPack(); });
+auto *NewResolvedPack =
+cast((*BPack)->getBinding());
+auto OldExprs = OldResolvedPack->getExprs();
+auto NewExprs = NewResolvedPack->getExprs();
zyn0217 wrote:
`assert(OldExprs.size() == NewExprs.size())`
https://github.com/llvm/llvm-project/pull/121417
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][P1061] Add stuctured binding packs (PR #121417)
https://github.com/zyn0217 commented: Thanks, this looks much better now. I went through another pass, focusing mostly on the pack expansion part. https://github.com/llvm/llvm-project/pull/121417 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][P1061] Add stuctured binding packs (PR #121417)
@@ -50,17 +50,29 @@ class CollectUnexpandedParameterPacksVisitor
auto *FTD = FD ? FD->getDescribedFunctionTemplate() : nullptr;
if (FTD && FTD->getTemplateParameters()->getDepth() >= DepthLimit)
return;
- } else if (getDepthAndIndex(ND).first >= DepthLimit)
+ } else if (auto *BD = dyn_cast(ND)) {
+Expr *E = BD->getBinding();
+if (auto *RP = dyn_cast_if_present(E)) {
+ addUnexpanded(RP);
+ return;
+}
zyn0217 wrote:
I was thinking if we can get rid of the `dyn_cast_if_present` thing..
Do you have a case where we ended up in `addUnexpanded` with a BindingDecl
whose binding isn't a ResolvedUnexpandedPackExpr?
https://github.com/llvm/llvm-project/pull/121417
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][P1061] Add stuctured binding packs (PR #121417)
@@ -1166,26 +1166,54 @@
TemplateDeclInstantiator::VisitTypeAliasTemplateDecl(TypeAliasTemplateDecl *D) {
Decl *TemplateDeclInstantiator::VisitBindingDecl(BindingDecl *D) {
auto *NewBD = BindingDecl::Create(SemaRef.Context, Owner, D->getLocation(),
-D->getIdentifier());
+D->getIdentifier(), D->getType());
NewBD->setReferenced(D->isReferenced());
SemaRef.CurrentInstantiationScope->InstantiatedLocal(D, NewBD);
+
return NewBD;
}
Decl *TemplateDeclInstantiator::VisitDecompositionDecl(DecompositionDecl *D) {
// Transform the bindings first.
+ // The transformed DD will have all of the concrete BindingDecls.
SmallVector NewBindings;
- for (auto *OldBD : D->bindings())
+ ResolvedUnexpandedPackExpr *OldResolvedPack = nullptr;
+ for (auto *OldBD : D->bindings()) {
+Expr *BindingExpr = OldBD->getBinding();
+if (auto *RP =
dyn_cast_if_present(BindingExpr))
+ OldResolvedPack = RP;
zyn0217 wrote:
We probably want to assert there's no more than one ResolvedUnexpandedPackExpr
around
https://github.com/llvm/llvm-project/pull/121417
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][P1061] Add stuctured binding packs (PR #121417)
@@ -15991,6 +15998,24 @@
TreeTransform::TransformFunctionParmPackExpr(FunctionParmPackExpr *E) {
return E;
}
+template
+ExprResult TreeTransform::TransformResolvedUnexpandedPackExpr(
+ResolvedUnexpandedPackExpr *E) {
+ bool ArgumentChanged = false;
+ SmallVector NewExprs;
+ if (TransformExprs(E->getExprs().begin(), E->getNumExprs(),
+ /*IsCall=*/false, NewExprs, &ArgumentChanged))
+return ExprError();
+
+ if (!AlwaysRebuild() && !ArgumentChanged)
+return E;
+
+ // NOTE: The type is just a superficial PackExpansionType
+ // that needs no substitution.
zyn0217 wrote:
Can we add a caveat to `ResolvedUnexpandedPackExpr` to highlight such a thing?
People might be confused about an expanded ResolvedUnexpandedPackExpr still
having an unexpanded PackExpansionType.
https://github.com/llvm/llvm-project/pull/121417
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][P1061] Add stuctured binding packs (PR #121417)
@@ -757,23 +775,40 @@ bool Sema::CheckParameterPacksForExpansion( bool HaveFirstPack = false; std::optional NumPartialExpansions; SourceLocation PartiallySubstitutedPackLoc; + typedef LocalInstantiationScope::DeclArgumentPack DeclArgumentPack; zyn0217 wrote: So now you can remove line 821 :) https://github.com/llvm/llvm-project/pull/121417 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][P1061] Add stuctured binding packs (PR #121417)
@@ -951,28 +959,125 @@ Sema::ActOnDecompositionDeclarator(Scope *S, Declarator &D, return New; } +// CheckBindingsCount +// - Checks the arity of the structured bindings +// - Creates the resolved pack expr if there is +//one + zyn0217 wrote: ```suggestion ``` https://github.com/llvm/llvm-project/pull/121417 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][P1061] Add stuctured binding packs (PR #121417)
@@ -1166,26 +1166,54 @@
TemplateDeclInstantiator::VisitTypeAliasTemplateDecl(TypeAliasTemplateDecl *D) {
Decl *TemplateDeclInstantiator::VisitBindingDecl(BindingDecl *D) {
auto *NewBD = BindingDecl::Create(SemaRef.Context, Owner, D->getLocation(),
-D->getIdentifier());
+D->getIdentifier(), D->getType());
NewBD->setReferenced(D->isReferenced());
SemaRef.CurrentInstantiationScope->InstantiatedLocal(D, NewBD);
+
return NewBD;
}
Decl *TemplateDeclInstantiator::VisitDecompositionDecl(DecompositionDecl *D) {
// Transform the bindings first.
+ // The transformed DD will have all of the concrete BindingDecls.
SmallVector NewBindings;
- for (auto *OldBD : D->bindings())
+ ResolvedUnexpandedPackExpr *OldResolvedPack = nullptr;
+ for (auto *OldBD : D->bindings()) {
+Expr *BindingExpr = OldBD->getBinding();
+if (auto *RP =
dyn_cast_if_present(BindingExpr))
+ OldResolvedPack = RP;
NewBindings.push_back(cast(VisitBindingDecl(OldBD)));
+ }
ArrayRef NewBindingArray = NewBindings;
- auto *NewDD = cast_or_null(
+ auto *NewDD = cast_if_present(
VisitVarDecl(D, /*InstantiatingVarTemplate=*/false, &NewBindingArray));
if (!NewDD || NewDD->isInvalidDecl())
for (auto *NewBD : NewBindings)
NewBD->setInvalidDecl();
+ if (OldResolvedPack) {
+// Mark the holding vars (if any) in the pack as instantiated since
+// they are created implicitly.
+auto Bindings = NewDD->bindings();
+auto BPack = std::find_if(
+Bindings.begin(), Bindings.end(),
+[](BindingDecl *D) -> bool { return D->isParameterPack(); });
zyn0217 wrote:
```suggestion
auto BPack = llvm::find_if(
Bindings, [](BindingDecl *D) { return D->isParameterPack(); });
```
https://github.com/llvm/llvm-project/pull/121417
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][P1061] Add stuctured binding packs (PR #121417)
@@ -1508,23 +1612,52 @@ static bool checkMemberDecomposition(Sema &S,
ArrayRef Bindings,
Qualifiers Q = DecompType.getQualifiers();
if (FD->isMutable())
Q.removeConst();
-B->setBinding(S.BuildQualifiedType(FD->getType(), Loc, Q), E.get());
+Walker.commitAndAdvance(S.BuildQualifiedType(FD->getType(), Loc, Q),
+E.get());
}
- if (I != Bindings.size())
-return DiagnoseBadNumberOfBindings();
-
return false;
}
+unsigned Sema::GetDecompositionElementCount(QualType DecompType) {
+ assert(!DecompType->isDependentType() && "expecting non-dependent type");
+ SourceLocation Loc = SourceLocation(); // FIXME
+ DecompType = DecompType.getNonReferenceType();
+ if (auto *CAT = Context.getAsConstantArrayType(DecompType))
+return CAT->getSize().getLimitedValue(UINT_MAX);
+ if (auto *VT = DecompType->getAs())
+return VT->getNumElements();
+ if (auto *CT = DecompType->getAs())
+return 2;
+ llvm::APSInt TupleSize(32);
+ if (IsTupleLike TL = isTupleLike(*this, Loc, DecompType, TupleSize);
+ TL == IsTupleLike::TupleLike)
+return (unsigned)TupleSize.getLimitedValue(UINT_MAX);
+
+ if (CXXRecordDecl *RD = DecompType->getAsCXXRecordDecl();
+ RD && !RD->isUnion()) {
+CXXCastPath BasePath;
+DeclAccessPair BasePair =
+findDecomposableBaseClass(*this, Loc, RD, BasePath);
+RD = cast_or_null(BasePair.getDecl());
+if (RD)
+ return llvm::count_if(
+ RD->fields(), [](FieldDecl *FD) { return !FD->isUnnamedBitField();
});
+ }
+
+ llvm_unreachable("unknown type for decomposition");
+}
+
void Sema::CheckCompleteDecompositionDeclaration(DecompositionDecl *DD) {
QualType DecompType = DD->getType();
// If the type of the decomposition is dependent, then so is the type of
// each binding.
if (DecompType->isDependentType()) {
-for (auto *B : DD->bindings())
- B->setType(Context.DependentTy);
+for (auto *B : DD->bindings()) {
+ if (B->getType().isNull())
+B->setType(Context.DependentTy);
+}
ricejasonf wrote:
Looking at this again, I see that I was avoiding replacing the
PackExpansionType here.
https://github.com/llvm/llvm-project/pull/121417
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][P1061] Add stuctured binding packs (PR #121417)
@@ -104,7 +104,6 @@ void CodeGenFunction::EmitDecl(const Decl &D) {
case Decl::Binding:
case Decl::UnresolvedUsingIfExists:
case Decl::HLSLBuffer:
-llvm_unreachable("Declaration should not be in declstmts!");
ricejasonf wrote:
I will just put this back in. It has to be an accident, or I do not remember
why I removed this. :sweat_smile:
https://github.com/llvm/llvm-project/pull/121417
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][P1061] Add stuctured binding packs (PR #121417)
@@ -237,7 +237,7 @@ bool Decl::isTemplateParameterPack() const {
}
bool Decl::isParameterPack() const {
- if (const auto *Var = dyn_cast(this))
+ if (const auto *Var = dyn_cast(this))
ricejasonf wrote:
They would still both just look for a PackExpansionType. There is no place
where a variable can be expanded that a binding cannot so I think it benefits
to have this in ValueDecl. See its use in SemaTemplateVariadic.cpp.
https://github.com/llvm/llvm-project/pull/121417
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][P1061] Add stuctured binding packs (PR #121417)
cor3ntin wrote: I plan to do another pass in the next couple of days, sorry for the delay https://github.com/llvm/llvm-project/pull/121417 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][P1061] Add stuctured binding packs (PR #121417)
https://github.com/zwuis edited https://github.com/llvm/llvm-project/pull/121417 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][P1061] Add stuctured binding packs (PR #121417)
https://github.com/zwuis edited https://github.com/llvm/llvm-project/pull/121417 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][P1061] Add stuctured binding packs (PR #121417)
@@ -237,7 +237,7 @@ bool Decl::isTemplateParameterPack() const {
}
bool Decl::isParameterPack() const {
- if (const auto *Var = dyn_cast(this))
+ if (const auto *Var = dyn_cast(this))
zwuis wrote:
My idea about this part of changes is adding `BindingDecl::isParameterPack()`
and
```cpp
if (const auto *BD = dyn_cast(this))
return BD->isParameterPack();
```
rather than changing `VarDecl::isParameterPack()` to
`ValueDecl::isParameterPack()`. WDYT?
https://github.com/llvm/llvm-project/pull/121417
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][P1061] Add stuctured binding packs (PR #121417)
https://github.com/zwuis commented: FYI Clang Static Analyzer (I built this branch locally) crashes when running with 'clang/test/SemaCXX/cxx2c-binding-pack.cpp'. My test command is `path/to/build/bin/clang-tidy --checks=clang-static-analyzer-core.BitwiseShift another/path/to/SemaCXX/cxx2c-binding-pack.cpp`. https://github.com/llvm/llvm-project/pull/121417 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][P1061] Add stuctured binding packs (PR #121417)
@@ -104,7 +104,6 @@ void CodeGenFunction::EmitDecl(const Decl &D) {
case Decl::Binding:
case Decl::UnresolvedUsingIfExists:
case Decl::HLSLBuffer:
-llvm_unreachable("Declaration should not be in declstmts!");
zwuis wrote:
Can we move some `case`s after this statement instead of removing `unreachable`?
https://github.com/llvm/llvm-project/pull/121417
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][P1061] Add stuctured binding packs (PR #121417)
https://github.com/zwuis edited https://github.com/llvm/llvm-project/pull/121417 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][P1061] Add stuctured binding packs (PR #121417)
ricejasonf wrote:
@erichkeane, thanks for the approval. Does that mean I should stop making
changes to this PR? I was messing with a more rangey interface for
`flat_bindings`, but for some reason the `llvm::concat` range was referring to
uninitialized memory when used as a temporary in a for loop (which even
pre-C++23 I could not see why this was the case). I am not asking anyone to
debug this, but I just wanted to know if this was the kind of "iterator" thing
that you were requesting.
```cpp
// Provide a flattened range to visit each binding.
auto flat_bindings() const {
llvm::ArrayRef Bindings = bindings();
llvm::ArrayRef PackExprs;
// Split the bindings into subranges split by the pack.
auto S1 = Bindings.take_until([](BindingDecl* BD) {
return BD->isParameterPack();
});
Bindings = Bindings.drop_front(S1.size());
if (!Bindings.empty()) {
PackExprs = Bindings.front()->getBindingPackExprs();
Bindings = Bindings.drop_front();
}
auto S2 = llvm::map_range(PackExprs, [](Expr* E) {
auto *DRE = cast(E);
return cast(DRE->getDecl());
});
return llvm::concat(S1, S2, Bindings);
}
```
I appreciate the people who have reviewed this PR as it did come with some of
its own legacy and cruft.
Let me know if you want me to address any of the remaining concerns or not.
https://github.com/llvm/llvm-project/pull/121417
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][P1061] Add stuctured binding packs (PR #121417)
https://github.com/erichkeane approved this pull request. I'm good here. Please let others make sure they are as well though. https://github.com/llvm/llvm-project/pull/121417 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][P1061] Add stuctured binding packs (PR #121417)
@@ -1965,3 +1965,52 @@ CXXFoldExpr::CXXFoldExpr(QualType T,
UnresolvedLookupExpr *Callee,
SubExprs[SubExpr::RHS] = RHS;
setDependence(computeDependence(this));
}
+
+ResolvedUnexpandedPackExpr::ResolvedUnexpandedPackExpr(SourceLocation BL,
+ QualType QT,
+ unsigned NumExprs)
+: Expr(ResolvedUnexpandedPackExprClass, QT, VK_PRValue, OK_Ordinary),
+ BeginLoc(BL), NumExprs(NumExprs) {
+ // C++ [temp.dep.expr]p3
+ // An id-expression is type-dependent if it is
+ //- associated by name lookup with a pack
+ setDependence(ExprDependence::TypeValueInstantiation |
+ExprDependence::UnexpandedPack);
+}
+
+ResolvedUnexpandedPackExpr *
+ResolvedUnexpandedPackExpr::CreateDeserialized(ASTContext &Ctx,
+ unsigned NumExprs) {
+ void *Mem = Ctx.Allocate(totalSizeToAlloc(NumExprs),
+ alignof(ResolvedUnexpandedPackExpr));
+ return new (Mem)
+ ResolvedUnexpandedPackExpr(SourceLocation(), QualType(), NumExprs);
+}
+
+ResolvedUnexpandedPackExpr *
+ResolvedUnexpandedPackExpr::Create(ASTContext &Ctx, SourceLocation BL,
+ QualType T, unsigned NumExprs) {
+ void *Mem = Ctx.Allocate(totalSizeToAlloc(NumExprs),
+ alignof(ResolvedUnexpandedPackExpr));
+ ResolvedUnexpandedPackExpr *New =
+ new (Mem) ResolvedUnexpandedPackExpr(BL, T, NumExprs);
+
+ auto Exprs = llvm::MutableArrayRef(New->getExprs(), New->getNumExprs());
+ std::fill(Exprs.begin(), Exprs.end(), nullptr);
+
+ return New;
+}
+
+ResolvedUnexpandedPackExpr *
+ResolvedUnexpandedPackExpr::Create(ASTContext &Ctx, SourceLocation BL,
+ QualType T, ArrayRef Exprs) {
+ auto *New = Create(Ctx, BL, T, Exprs.size());
+ std::copy(Exprs.begin(), Exprs.end(), New->getExprs());
erichkeane wrote:
IIRC I looked it up at one point and found that std::copy into uninitialized
data (or perhaps std::fill as well), but I cannot for the life of me remember
what it was. Perhaps it is just 'dtor' related stuff (which makes this not
matter for pointers?) but the code I wrote is pointers-only too, so IDK why it
became a bit of my personal cargo-cult.
https://github.com/llvm/llvm-project/pull/121417
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][P1061] Add stuctured binding packs (PR #121417)
https://github.com/ricejasonf edited https://github.com/llvm/llvm-project/pull/121417 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][P1061] Add stuctured binding packs (PR #121417)
@@ -1965,3 +1965,52 @@ CXXFoldExpr::CXXFoldExpr(QualType T,
UnresolvedLookupExpr *Callee,
SubExprs[SubExpr::RHS] = RHS;
setDependence(computeDependence(this));
}
+
+ResolvedUnexpandedPackExpr::ResolvedUnexpandedPackExpr(SourceLocation BL,
+ QualType QT,
+ unsigned NumExprs)
+: Expr(ResolvedUnexpandedPackExprClass, QT, VK_PRValue, OK_Ordinary),
+ BeginLoc(BL), NumExprs(NumExprs) {
+ // C++ [temp.dep.expr]p3
+ // An id-expression is type-dependent if it is
+ //- associated by name lookup with a pack
+ setDependence(ExprDependence::TypeValueInstantiation |
+ExprDependence::UnexpandedPack);
+}
+
+ResolvedUnexpandedPackExpr *
+ResolvedUnexpandedPackExpr::CreateDeserialized(ASTContext &Ctx,
+ unsigned NumExprs) {
+ void *Mem = Ctx.Allocate(totalSizeToAlloc(NumExprs),
+ alignof(ResolvedUnexpandedPackExpr));
+ return new (Mem)
+ ResolvedUnexpandedPackExpr(SourceLocation(), QualType(), NumExprs);
+}
+
+ResolvedUnexpandedPackExpr *
+ResolvedUnexpandedPackExpr::Create(ASTContext &Ctx, SourceLocation BL,
+ QualType T, unsigned NumExprs) {
+ void *Mem = Ctx.Allocate(totalSizeToAlloc(NumExprs),
+ alignof(ResolvedUnexpandedPackExpr));
+ ResolvedUnexpandedPackExpr *New =
+ new (Mem) ResolvedUnexpandedPackExpr(BL, T, NumExprs);
+
+ auto Exprs = llvm::MutableArrayRef(New->getExprs(), New->getNumExprs());
+ std::fill(Exprs.begin(), Exprs.end(), nullptr);
+
+ return New;
+}
+
+ResolvedUnexpandedPackExpr *
+ResolvedUnexpandedPackExpr::Create(ASTContext &Ctx, SourceLocation BL,
+ QualType T, ArrayRef Exprs) {
+ auto *New = Create(Ctx, BL, T, Exprs.size());
+ std::copy(Exprs.begin(), Exprs.end(), New->getExprs());
ricejasonf wrote:
I suppose it could. The other Create function it is calling here is using
`std::fill` which could probably be `std::uninitialized_fill`.
https://github.com/llvm/llvm-project/pull/121417
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][P1061] Add stuctured binding packs (PR #121417)
https://github.com/ricejasonf edited https://github.com/llvm/llvm-project/pull/121417 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
