LWG-3211 std::tuple<> should be trivially constructible

[futuarmo]

LWG-3211 std::tuple<> should be trivially constructible

Fixes #1459

[miscco]

Thanks for providing the patch.

However, there are other things to do:

1. The LWG issue only applies to C++20 so you should guard it with _HAS_CXX20
2. We need to test this. I would propose the tuple_cat test. Again, you should test pre C++20 and C++20 modes
3. Given the comment on the default constructor we should get a maintainers opinion on ABI

[futuarmo]

Thanks for providing the patch.

However, there are other things to do:

1. The LWG issue only applies to C++20 so you should guard it with `_HAS_CXX20`

2. We need to test this. I would propose the tuple_cat test. Again, you should test pre C++20 and C++20 modes

3. Given the comment on the default constructor we should get a maintainers opinion on ABI

Thanks for useful notice. I'l fix everything and will be more attentive in future

[futuarmo]

1. The LWG issue only applies to C++20 so you should guard it with `_HAS_CXX20`

Doesn't it depend of implementation is tuple<> constructor trivial or not? Potentially, if it won't break ABI, can we retain default constructor for previous standards too (since C++11)? (it's just interesting for me)

[miscco]

Thanks for useful notice. I'l fix everything and will be more attentive in future

No need to feel bad. This is tricky stuff with lots of hidden convention and unknown hooks attached. You are doing fine ;)

1. The LWG issue only applies to C++20 so you should guard it with `_HAS_CXX20`

Doesn't it depend of implementation is tuple<> constructor trivial or not? Potentially, if it won't break ABI, can we retain default constructor for previous standards too (since C++11)? (it's just interesting for me)

So the question is whether we can change properties of the class. Previously is_trivially_default_constructible_v<tuple<>> would not hold true.

We would change the behavior of previousstandard modes, which often is tricky, but not unheard of. I would definitely defer to a maintainer here.

This is also the reason you often see things like

#if _HAS_CXX20

#else // ^^^ _HAS_CXX20 / vvv !_HAS_CXX20 vvv

#endif // !_HAS_CXX20

[StephanTLavavej]

We follow different conventions for features and LWG issues.

For features, we guard them with _HAS_CXX20 unless there is a compelling reason to implement them unconditionally. For standalone features (i.e. adding new classes/functions), there is generally never such a reason. (If we want to use things like bit_cast or remove_cvref_t in earlier Standard modes, we'll instead add _Ugly versions that do the same thing.) Ditto for things like new typedefs in old classes (e.g. shared_ptr::weak_type). However, when a feature makes an invasive change to existing machinery, and the change is non-source-breaking or close to it, we'll consider implementing it unconditionally, because having two different versions of the same machinery is complicated and difficult to maintain. P1165R1 "Consistently Propagating Stateful Allocators In basic_string's operator+()" was a recent example.

For LWG issues, we implement them unconditionally, unless we discover that they have significant source-breaking impact. That's very rare - LWG-2221 "No formatted output operator for nullptr" is the only time in recent memory that that's happened (we had to guard it for C++17 because it broke too much code in C++14). LWG-2911 "An is_aggregate type trait is needed" was also guarded for C++17 but that's because it was a micro-feature that sneaked in through the issues list.

Here, if changing tuple<>'s default constructor is ABI-safe, I believe we should do so unconditionally - as @miscco mentioned, it is observable through is_trivially_default_constructible_v, but that should be extremely unlikely to break source code (at most I would expect libcxx's tests to notice), and it is the direct point of the LWG issue.

As the comment for tuple<>'s copy constructor notes, this is a relatively scary area - I broke ABI and had to revert it when I attempted to default the copy ctor (IIRC it was around VS 2017 15.5). As far as I know, this applies only to the copy ctor which affects how classes are passed by value, and should not affect the default ctor. However, we'll want to carefully investigate.

[StephanTLavavej]

Looks like the only thing this PR needs is adding a line to a test somewhere, verifying that tuple<> is trivially default constructible.

[miscco]

Thanks a lot for fixing this!

[StephanTLavavej]

Thanks, this looks good to me! I'll push an essentially stylistic change to the test.

[StephanTLavavej]

I've verified that this preserves ABI. 😻 Full test case, compiled with x64:

C:\Temp>type meow.cpp

struct CurrentTuple {
    constexpr CurrentTuple() noexcept {}
    constexpr CurrentTuple(const CurrentTuple&) noexcept {}
};

struct ThisPR {
    constexpr ThisPR() noexcept = default;
    constexpr ThisPR(const ThisPR&) noexcept {}
};

struct BrokeABI {
    constexpr BrokeABI() noexcept {}
    constexpr BrokeABI(const BrokeABI&) noexcept = default;
};

template <typename Base>
struct TwoInts : Base {
    int x;
    int y;
};

int func1(TwoInts<CurrentTuple> a) {
    return a.x + a.y;
}
int func2(TwoInts<ThisPR> a) {
    return a.x + a.y;
}
int func3(TwoInts<BrokeABI> a) {
    return a.x + a.y;
}

int main() {
    TwoInts<CurrentTuple> current{};
    TwoInts<ThisPR> thispr{};
    TwoInts<BrokeABI> brokeabi{};
    func1(current);
    func2(thispr);
    func3(brokeabi);
}

C:\Temp>cl /EHsc /nologo /W4 /Zc:inline /c /FAs meow.cpp
meow.cpp

C:\Temp>

Inspecting meow.asm reveals how the codegen for constructing TwoInts<ThisPR> improves, but the codegen for calling func2(TwoInts<ThisPR>) is unchanged relative to func1(TwoInts<CurrentTuple>). In contrast, func3(TwoInts<BrokeABI>) demonstrates how I damaged tuple<> long ago. My original notes for that bug:

[backported to 15.6.0] Fix tuple bincompat. Checkin 1662809 on 6/22/2017 (which first shipped in VS 2017 15.5) removed the empty tuple's copy ctor in order to improve conformance. Unfortunately, this affects binary compatibility. On x64, this affects the calling convention of functions taking tuples by value (not by reference) when they're 8 bytes or less. Tuples of scalars AND tuples of references (e.g. tuple<int>, tuple<int, int>, tuple<long long>, tuple<int&>) are affected. It also appears that this affects map indirectly. map piecewise-constructs pair which takes tuples by value. I've manually verified that this changes the codegen back to what it used to be.

[StephanTLavavej]

Thanks for implementing this LWG issue and improving tuple codegen! 🚀 😸