On Fri, 11 Oct 2024 at 20:35, Jonathan Wakely <jwak...@redhat.com> wrote:
>
> On Fri, 11 Oct 2024 at 20:33, Jonathan Wakely <jwak...@redhat.com> wrote:
> >
> > On Sat, 21 Sept 2024 at 10:43, Giuseppe D'Angelo
> > <giuseppe.dang...@kdab.com> wrote:
> > >
> > > Hello,
> > >
> > > The attached patch modifies std::atomic's primary template. The goal is
> > > to improve compatibility with Clang, while also possibly making it more
> > > complaint with the changes introduced by P0883 / C++20.
> > >
> > > Simplifying, std::atomic has a `T t = T()` NDSMI and a defaulted default
> > > constructor. The crux of the problem is that Clang seems to be stricter
> > > than GCC when that constructor is considered / instantiated.
> > >
> > > Given a non-default constructible type NDC, doing something like
> > >
> > > constexpr bool b = std::is_default_constructible_v<std::atomic<NDC>>;
> > >
> > > causes a hard error on Clang because it will "see" the call to `NDC()`
> > > in the NDSMI. The code is instead accepted by GCC. This hard error will
> > > happen anywhere one "mentions" std::atomic<NDC>'s default constructor,
> > > for instance in libstdc++'s C++20 std::pair implementation (uses them in
> > > the explicit(bool) bits). You can play with this here:
> > >
> > > https://gcc.godbolt.org/z/xcr4zK8hx
> > >
> > > PR116769 argues that Clang's behavior is the correct one here, so this
> > > patch improves compat with Clang by removing the defaulted default
> > > constructor.
> >
> > GCC's behaviour seems much more useful.
> >
> > > A related issue is: what's the value of `b` above? std::atomic's default
> > > constructor is not constrained, so it should be `true`. Right now we're
> > > reporting `false` instead.
> >
> > Good, that's the correct answer :-)
> > I don't understand why anybody would want the NSDMI to be ignored and
> > give the wrong answer, or be instantiated and give a hard error.
> >
> >
> > > Thoughts?
> >
> > Your patch changes the value of
> > is_trivially_default_constructible_v<std::atomic<int>> for
> > C++11/14/17.
> >
> > Currently that is true for <= 17 and true for >= 20. You patch makes
> > it always false.
> >
> > If we did this instead, I think all compilers would handle it
> > correctly and we wouldn't change any behaviour for C++17 down:
> >
> > atomic() = default;
> > #ifdef __cpp_concepts >= 202002
>
> We might not require 202002 here, that's the value for P0848R3 and we
> might not need that to conditionally delete the default ctor. I don't
> remember the details.
>
> It might be sufficient to just check that __cpp_concepts is defined.
>
> > atomic() requires (!std::is_constructible_v<_Tp>) = delete;
> > #endif
> >
> > For C++17 there's no NSDMI and the default constructor does the right
> > thing (getting deleted if T is not default constructible).
> > For C++20 the default constructor is deleted if the NSDMI would be
> > ill-formed, which is consistent with the C++17 behaviour.
> > The triviality of the constructor is unchanged.
Or alternatively, and arguably simpler, we could get rid of the ugly
_GLIBCXX20_INIT macro and just do this:
diff --git a/libstdc++-v3/include/bits/version.def
b/libstdc++-v3/include/bits/version.def
index 6783f9aa2a5..942a9c342f2 100644
--- a/libstdc++-v3/include/bits/version.def
+++ b/libstdc++-v3/include/bits/version.def
@@ -766,6 +766,7 @@ ftms = {
values = {
v = 201911;
cxxmin = 20;
+ extra_cond = "__cpp_concepts";
};
};
diff --git a/libstdc++-v3/include/std/atomic b/libstdc++-v3/include/std/atomic
index c0568d3320b..a555624a2c9 100644
--- a/libstdc++-v3/include/std/atomic
+++ b/libstdc++-v3/include/std/atomic
@@ -189,14 +189,6 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
#endif // __cpp_lib_atomic_wait
};
-/// @cond undocumented
-#if __cpp_lib_atomic_value_initialization
-# define _GLIBCXX20_INIT(I) = I
-#else
-# define _GLIBCXX20_INIT(I)
-#endif
-/// @endcond
-
/**
* @brief Generic atomic type, primary class template.
*
@@ -216,7 +208,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
static constexpr int _S_alignment
= _S_min_alignment > alignof(_Tp) ? _S_min_alignment : alignof(_Tp);
- alignas(_S_alignment) _Tp _M_i _GLIBCXX20_INIT(_Tp());
+ alignas(_S_alignment) _Tp _M_i;
static_assert(__is_trivially_copyable(_Tp),
"std::atomic requires a trivially copyable type");
@@ -232,7 +224,13 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
#endif
public:
+#if __cpp_lib_atomic_value_initialization
+ atomic() noexcept (is_nothrow_default_constructible_v<_Tp>)
+ requires (is_default_constructible_v<_Tp>)
+ : _M_i() { }
+#else
atomic() = default;
+#endif
~atomic() noexcept = default;
atomic(const atomic&) = delete;
atomic& operator=(const atomic&) = delete;
This more closely matches what's in the standard, except that we have
a requires-clause on the default constructor. It seems like a defect
that the standard doesn't do this, instead requiring
is_constructible<atomic<NDC>> to be true, but Mandating an error if
you try to use it.
