Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Clang C++20 Feature: P0960R3 -Allow initializing aggregates from a parenthesized list of values #54040

Closed
tstellar opened this issue Feb 24, 2022 · 10 comments
Assignees
Labels
c++20 clang:frontend Language frontend issues, e.g. anything involving "Sema"
Milestone

Comments

@tstellar
Copy link
Collaborator

tstellar commented Feb 24, 2022

Blocks #46701


id: P0960R3
paper: https://wg21.link/p0960r3

@tstellar tstellar added this to the Clang C++20 milestone Feb 24, 2022
@llvmbot
Copy link
Collaborator

llvmbot commented Feb 24, 2022

@llvm/issue-subscribers-c-20

@llvmbot
Copy link
Collaborator

llvmbot commented Jun 10, 2022

@llvm/issue-subscribers-clang-frontend

@0x59616e 0x59616e self-assigned this Jun 18, 2022
@0x59616e
Copy link
Contributor

Candidate patch : https://reviews.llvm.org/D129531

@danakj
Copy link
Contributor

danakj commented Dec 18, 2022

Thank you for working on this. One thing that was not fixed is templated classes.

https://godbolt.org/z/6a6Ws674o

The following works in GCC and MSVC but not on Clang trunk yet:

struct S {
    int i;
    float j;
};

template <class T>
struct ST {
    T t;
};

int main() {
    auto s = S(1, 2.f);  // works
    auto st = ST(1);  // no work
}

danakj added a commit to danakj/subspace that referenced this issue Dec 19, 2022
llvm/llvm-project#54040 has resolved the issue
unless the class members are template typed.
@alanzhao1
Copy link
Contributor

That specific example is out of scope of P0960R3 because it's covered by two other C++20 feature requests - P1816R0 and P2082R1, both of which track class template argument deduction for aggregates. Support for this in Clang is tracked in #54049 and #54050

@danakj
Copy link
Contributor

danakj commented Dec 19, 2022

Thanks, one more thing I ran into: A destructor breaks the aggregate init for Clang, though not for MSVC and GCC.

https://godbolt.org/z/sbj8hTKWb

struct Raii final {
    ~Raii() {}
    int i;
};

int main() {
    auto r = Raii(2);
}
<source>:7:14: error: no matching conversion for functional-style cast from 'int' to 'Raii'
    auto r = Raii(2);
             ^~~~~~
<source>:1:8: note: candidate constructor (the implicit copy constructor) not viable: no known conversion from 'int' to 'const Raii' for 1st argument
struct Raii final {
       ^
<source>:1:8: note: candidate constructor (the implicit default constructor) not viable: requires 0 arguments, but 1 was provided
1 error generated.

danakj added a commit to danakj/subspace that referenced this issue Dec 19, 2022
llvm/llvm-project#54040 has resolved the issue
of aggregates requiring an explicit constructor for parenthesis
initialization. But there's some more cases where aggregate init fails
on Clang.

* If there's a destructor present, the aggregate constructor appears to
  not be generated. I believe this is still
  llvm/llvm-project#54040.
* If the class is a template, CTAD fails to apply and aggregate
  initialization breaks as a result. This is
  llvm/llvm-project#54050.
@danakj
Copy link
Contributor

danakj commented Dec 19, 2022

Another issue is caused somehow by a concept-guarded conversion operator on the type of the aggregate's member.

https://godbolt.org/z/sMY7WfGhx

#include <concepts>

struct i32 {
    int i = 0;
    
    template <std::same_as<int> T>
    operator T() const { return i; }  // breaks init of S()
};

int main() {
    struct S {
        i32 i;
    };

    auto s1 = S(i32(2));
}
<source>:15:15: error: no matching conversion for functional-style cast from 'i32' to 'S'
    auto s1 = S(i32(2));
              ^~~~~~~~
<source>:11:12: note: candidate constructor (the implicit copy constructor) not viable: no known conversion from 'i32' to 'const S' for 1st argument
    struct S {
           ^
<source>:11:12: note: candidate constructor (the implicit move constructor) not viable: no known conversion from 'i32' to 'S' for 1st argument
    struct S {
           ^
<source>:11:12: note: candidate constructor (the implicit default constructor) not viable: requires 0 arguments, but 1 was provided
<source>:7:5: note: candidate template ignored: constraints not satisfied [with T = S]
    operator T() const { return i; }  // breaks init of S()
    ^
<source>:6:15: note: because 'std::same_as<S, int>' evaluated to false
    template <std::same_as<int> T>
              ^
/opt/compiler-explorer/gcc-snapshot/lib/gcc/x86_64-linux-gnu/13.0.0/../../../../include/c++/13.0.0/concepts:63:9: note: because '__detail::__same_as<S, int>' evaluated to false
      = __detail::__same_as<_Tp, _Up> && __detail::__same_as<_Up, _Tp>;
        ^
/opt/compiler-explorer/gcc-snapshot/lib/gcc/x86_64-linux-gnu/13.0.0/../../../../include/c++/13.0.0/concepts:57:27: note: because 'std::is_same_v<S, int>' evaluated to false
      concept __same_as = std::is_same_v<_Tp, _Up>;
                          ^
1 error generated.

danakj added a commit to danakj/subspace that referenced this issue Dec 19, 2022
llvm/llvm-project#54040 has resolved the issue
of aggregates requiring an explicit constructor for parenthesis
initialization. But there's some more cases where aggregate init fails
on Clang.

* If there's a destructor present, the aggregate constructor appears to
  not be generated. I believe this is still
  llvm/llvm-project#54040.
* If the class is a template, CTAD fails to apply and aggregate
  initialization breaks as a result. This is
  llvm/llvm-project#54050.
danakj added a commit to danakj/subspace that referenced this issue Dec 19, 2022
llvm/llvm-project#54040 has resolved the issue
of aggregates requiring an explicit constructor for parenthesis
initialization. But there's some more cases where aggregate init fails
on Clang.

* If there's a destructor present, the aggregate constructor appears to
  not be generated. I believe this is still
  llvm/llvm-project#54040.
* If the class is a template, CTAD fails to apply and aggregate
  initialization breaks as a result. This is
  llvm/llvm-project#54050.
danakj added a commit to danakj/subspace that referenced this issue Dec 19, 2022
llvm/llvm-project#54040 has resolved the issue
of aggregates requiring an explicit constructor for parenthesis
initialization. But there's some more cases where aggregate init fails
on Clang.

* If there's a destructor present, the aggregate constructor appears to
  not be generated. I believe this is still
  llvm/llvm-project#54040.
* If the class is a template, CTAD fails to apply and aggregate
  initialization breaks as a result. This is
  llvm/llvm-project#54050.
danakj added a commit to danakj/subspace that referenced this issue Dec 19, 2022
llvm/llvm-project#54040 has resolved the issue
of aggregates requiring an explicit constructor for parenthesis
initialization. But there's some more cases where aggregate init fails
on Clang.

* If there's a destructor present, the aggregate constructor appears to
  not be generated. I believe this is still
  llvm/llvm-project#54040.
* If the class is a template, CTAD fails to apply and aggregate
  initialization breaks as a result. This is
  llvm/llvm-project#54050.
danakj added a commit to chromium/subspace that referenced this issue Dec 19, 2022
llvm/llvm-project#54040 has resolved the issue
of aggregates requiring an explicit constructor for parenthesis
initialization. But there's some more cases where aggregate init fails
on Clang.

* If there's a destructor present, the aggregate constructor appears to
  not be generated. I believe this is still
  llvm/llvm-project#54040.
* If the class is a template, CTAD fails to apply and aggregate
  initialization breaks as a result. This is
  llvm/llvm-project#54050.
@alanzhao1
Copy link
Contributor

The previous two issues should be resolved by https://reviews.llvm.org/D140327

@danakj
Copy link
Contributor

danakj commented Dec 20, 2022

Awesome, thank you

alanzhao1 added a commit that referenced this issue Dec 21, 2022
Previously, we would only attempt to perform a parenthesized aggregate
initialization if constructor initialization failed for only the default
constructor, default copy constructor, and default move constructor. The
original intent of this logic was to reject initializing objects that
have failed resolving a user-defined constructor. However, this check is
redundant because we check for isAggregate() before attempting to
perform a parenthesized aggregate initialization, and classes that have
user-defined or user-declared constructors are not aggregates.
Furthermore, this check is too restrictive - the following valid
examples fail:
* Aggregate class with user-defined destructor - fails because default
  move constructors are not generated for classes with user-defined
  destructors
  (#54040 (comment))
* Concept-guarded conversion operator on an aggregate's member:
  (#54040 (comment))

The solution therefore is to remove this logic; existing tests still
pass, and the previously failing examples now compile.

Reviewed By: ilya-biryukov

Differential Revision: https://reviews.llvm.org/D140327
CarlosAlbertoEnciso pushed a commit to SNSystems/llvm-debuginfo-analyzer that referenced this issue Dec 25, 2022
Previously, we would only attempt to perform a parenthesized aggregate
initialization if constructor initialization failed for only the default
constructor, default copy constructor, and default move constructor. The
original intent of this logic was to reject initializing objects that
have failed resolving a user-defined constructor. However, this check is
redundant because we check for isAggregate() before attempting to
perform a parenthesized aggregate initialization, and classes that have
user-defined or user-declared constructors are not aggregates.
Furthermore, this check is too restrictive - the following valid
examples fail:
* Aggregate class with user-defined destructor - fails because default
  move constructors are not generated for classes with user-defined
  destructors
  (llvm/llvm-project#54040 (comment))
* Concept-guarded conversion operator on an aggregate's member:
  (llvm/llvm-project#54040 (comment))

The solution therefore is to remove this logic; existing tests still
pass, and the previously failing examples now compile.

Reviewed By: ilya-biryukov

Differential Revision: https://reviews.llvm.org/D140327
@alanzhao1
Copy link
Contributor

Reopening as this was reverted by 4e02ff2 due to #59675

@alanzhao1 alanzhao1 reopened this Jan 5, 2023
@alanzhao1 alanzhao1 assigned alanzhao1 and unassigned 0x59616e Jan 5, 2023
danakj added a commit to danakj/subspace that referenced this issue Jan 6, 2023
…ort (chromium#121)

llvm/llvm-project#54040 has been reverted due to a
compiler crash, so we must restore all our workaround constructors for it.
danakj added a commit to danakj/subspace that referenced this issue Jan 6, 2023
…ort (chromium#121)

llvm/llvm-project#54040 has been reverted due to a
compiler crash, so we must restore all our workaround constructors for it.
danakj added a commit to chromium/subspace that referenced this issue Jan 6, 2023
…ort (#121)

llvm/llvm-project#54040 has been reverted due to a
compiler crash, so we must restore all our workaround constructors for it.
malavikasamak pushed a commit to swiftlang/llvm-project that referenced this issue Jan 24, 2023
This commit relands the patches for implementing P0960R3 and P1975R0,
which describe initializing aggregates via a parenthesized list.

The relanded commits are:

* 40c5215 - P0960R3 and P1975R0: Allow initializing aggregates from
  a parenthesized list of values
* c77a91b - Remove overly restrictive aggregate paren init logic
* 32d7aae - Fix a clang crash on invalid code in C++20 mode

This patch also fixes a crash in the original implementation.
Previously, if the input tried to call an implicitly deleted copy or
move constructor of a union, we would then try to initialize the union
by initializing it's first element with a reference to a union. This
behavior is incorrect (we should fail to initialize) and if the type of
the first element has a constructor with a single template typename
parameter, then Clang will explode. This patch fixes that issue by
checking that constructor overload resolution did not result in a
deleted function before attempting parenthesized aggregate
initialization.

Additionally, this patch also includes D140159, which contains some
minor fixes made in response to code review comments in the original
implementation that were made after that patch was submitted.

Co-authored-by: Sheng <ox59616e@gmail.com>

Fixes llvm#54040, Fixes llvm#59675

Reviewed By: ilya-biryukov

Differential Revision: https://reviews.llvm.org/D141546

(cherry picked from commit 95a4c0c)
veselypeta pushed a commit to veselypeta/cherillvm that referenced this issue Jun 10, 2024
Previously, we would only attempt to perform a parenthesized aggregate
initialization if constructor initialization failed for only the default
constructor, default copy constructor, and default move constructor. The
original intent of this logic was to reject initializing objects that
have failed resolving a user-defined constructor. However, this check is
redundant because we check for isAggregate() before attempting to
perform a parenthesized aggregate initialization, and classes that have
user-defined or user-declared constructors are not aggregates.
Furthermore, this check is too restrictive - the following valid
examples fail:
* Aggregate class with user-defined destructor - fails because default
  move constructors are not generated for classes with user-defined
  destructors
  (llvm/llvm-project#54040 (comment))
* Concept-guarded conversion operator on an aggregate's member:
  (llvm/llvm-project#54040 (comment))

The solution therefore is to remove this logic; existing tests still
pass, and the previously failing examples now compile.

Reviewed By: ilya-biryukov

Differential Revision: https://reviews.llvm.org/D140327
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++20 clang:frontend Language frontend issues, e.g. anything involving "Sema"
Projects
Status: Done
Development

No branches or pull requests

7 participants