From: Jonathan Wakely <jwakely@redhat.com>
To: Jakub Jelinek <jakub@redhat.com>
Cc: gcc-patches@gcc.gnu.org, libstdc++@gcc.gnu.org
Subject: Re: [PATCH] libstdc++-v3: <complex> support for extended floating point types
Date: Mon, 31 Oct 2022 17:04:58 +0000 [thread overview]
Message-ID: <CACb0b4mkhTnWUPOU+BEh5f3TrB9zccwhE28V_g_pLdKzCG69ng@mail.gmail.com> (raw)
In-Reply-To: <Y1/+faSCyvTztVBH@tucnak>
On Mon, 31 Oct 2022 at 16:57, Jakub Jelinek <jakub@redhat.com> wrote:
>
> On Mon, Oct 31, 2022 at 10:26:11AM +0000, Jonathan Wakely wrote:
> > > --- libstdc++-v3/include/std/complex.jj 2022-10-21 08:55:43.037675332 +0200
> > > +++ libstdc++-v3/include/std/complex 2022-10-21 17:05:36.802243229 +0200
> > > @@ -142,8 +142,14 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> > >
> > > /// Converting constructor.
> > > template<typename _Up>
> > > +#if __cplusplus > 202002L
> > > + explicit(!requires(_Up __u) { _Tp{__u}; })
> > > + constexpr complex(const complex<_Up>& __z)
> > > + : _M_real(_Tp(__z.real())), _M_imag(_Tp(__z.imag())) { }
> >
> > Do these casts to _Tp do anything? The _M_real and _M_imag members are
> > already of type _Tp and we're using () initialization not {} so
> > there's no narrowing concern. _M_real(__z.real()) is already an
> > explicit conversion from decltype(__z.real()) to decltype(_M_real) so
> > the extra _Tp is redundant.
>
> If I use just
> : _M_real(__z.real()), _M_imag(__z.imag()) { }
> then without -Wsystem-headers there are no regressions, but compiling
> g++.dg/cpp23/ext-floating12.C with additional -Wsystem-headers
> -pedantic-errors results in lots of extra errors on that line:
> .../x86_64-pc-linux-gnu/libstdc++-v3/include/complex:149:27: error: converting to ‘_Float32’ from ‘double’ with greater conversion rank
> .../x86_64-pc-linux-gnu/libstdc++-v3/include/complex:149:48: error: converting to ‘_Float32’ from ‘double’ with greater conversion rank
> .../x86_64-pc-linux-gnu/libstdc++-v3/include/complex:149:27: error: converting to ‘_Float32’ from ‘long double’ with greater conversion rank
> .../x86_64-pc-linux-gnu/libstdc++-v3/include/complex:149:48: error: converting to ‘_Float32’ from ‘long double’ with greater conversion rank
> .../x86_64-pc-linux-gnu/libstdc++-v3/include/complex:149:27: error: converting to ‘_Float32’ from ‘_Float64’ with greater conversion rank
> .../x86_64-pc-linux-gnu/libstdc++-v3/include/complex:149:48: error: converting to ‘_Float32’ from ‘_Float64’ with greater conversion rank
> .../x86_64-pc-linux-gnu/libstdc++-v3/include/complex:149:27: error: converting to ‘_Float32’ from ‘_Float128’ with greater conversion rank
> .../x86_64-pc-linux-gnu/libstdc++-v3/include/complex:149:48: error: converting to ‘_Float32’ from ‘_Float128’ with greater conversion rank
> .../x86_64-pc-linux-gnu/libstdc++-v3/include/complex:149:27: error: converting to ‘_Float64’ from ‘long double’ with greater conversion rank
> .../x86_64-pc-linux-gnu/libstdc++-v3/include/complex:149:48: error: converting to ‘_Float64’ from ‘long double’ with greater conversion rank
> .../x86_64-pc-linux-gnu/libstdc++-v3/include/complex:149:27: error: converting to ‘_Float64’ from ‘_Float128’ with greater conversion rank
> .../x86_64-pc-linux-gnu/libstdc++-v3/include/complex:149:48: error: converting to ‘_Float64’ from ‘_Float128’ with greater conversion rank
> .../x86_64-pc-linux-gnu/libstdc++-v3/include/complex:149:27: error: converting to ‘_Float16’ from ‘float’ with greater conversion rank
> .../x86_64-pc-linux-gnu/libstdc++-v3/include/complex:149:48: error: converting to ‘_Float16’ from ‘float’ with greater conversion rank
> .../x86_64-pc-linux-gnu/libstdc++-v3/include/complex:149:27: error: converting to ‘_Float16’ from ‘double’ with greater conversion rank
> .../x86_64-pc-linux-gnu/libstdc++-v3/include/complex:149:48: error: converting to ‘_Float16’ from ‘double’ with greater conversion rank
> .../x86_64-pc-linux-gnu/libstdc++-v3/include/complex:149:27: error: converting to ‘_Float16’ from ‘long double’ with greater conversion rank
> .../x86_64-pc-linux-gnu/libstdc++-v3/include/complex:149:48: error: converting to ‘_Float16’ from ‘long double’ with greater conversion rank
> .../x86_64-pc-linux-gnu/libstdc++-v3/include/complex:149:27: error: converting to ‘_Float16’ from ‘_Float32’ with greater conversion rank
> .../x86_64-pc-linux-gnu/libstdc++-v3/include/complex:149:48: error: converting to ‘_Float16’ from ‘_Float32’ with greater conversion rank
> .../x86_64-pc-linux-gnu/libstdc++-v3/include/complex:149:27: error: converting to ‘_Float16’ from ‘_Float64’ with greater conversion rank
> .../x86_64-pc-linux-gnu/libstdc++-v3/include/complex:149:48: error: converting to ‘_Float16’ from ‘_Float64’ with greater conversion rank
> .../x86_64-pc-linux-gnu/libstdc++-v3/include/complex:149:27: error: converting to ‘_Float16’ from ‘_Float128’ with greater conversion rank
> .../x86_64-pc-linux-gnu/libstdc++-v3/include/complex:149:48: error: converting to ‘_Float16’ from ‘_Float128’ with greater conversion rank
> .../x86_64-pc-linux-gnu/libstdc++-v3/include/complex:149:27: error: converting to ‘_Float16’ from ‘__bf16’ with unordered conversion ranks
> .../x86_64-pc-linux-gnu/libstdc++-v3/include/complex:149:48: error: converting to ‘_Float16’ from ‘__bf16’ with unordered conversion ranks
> .../x86_64-pc-linux-gnu/libstdc++-v3/include/complex:149:27: error: converting to ‘__bf16’ from ‘float’ with greater conversion rank
> .../x86_64-pc-linux-gnu/libstdc++-v3/include/complex:149:48: error: converting to ‘__bf16’ from ‘float’ with greater conversion rank
> .../x86_64-pc-linux-gnu/libstdc++-v3/include/complex:149:27: error: converting to ‘__bf16’ from ‘double’ with greater conversion rank
> .../x86_64-pc-linux-gnu/libstdc++-v3/include/complex:149:48: error: converting to ‘__bf16’ from ‘double’ with greater conversion rank
> .../x86_64-pc-linux-gnu/libstdc++-v3/include/complex:149:27: error: converting to ‘__bf16’ from ‘long double’ with greater conversion rank
> .../x86_64-pc-linux-gnu/libstdc++-v3/include/complex:149:48: error: converting to ‘__bf16’ from ‘long double’ with greater conversion rank
> .../x86_64-pc-linux-gnu/libstdc++-v3/include/complex:149:27: error: converting to ‘__bf16’ from ‘_Float32’ with greater conversion rank
> .../x86_64-pc-linux-gnu/libstdc++-v3/include/complex:149:48: error: converting to ‘__bf16’ from ‘_Float32’ with greater conversion rank
> .../x86_64-pc-linux-gnu/libstdc++-v3/include/complex:149:27: error: converting to ‘__bf16’ from ‘_Float64’ with greater conversion rank
> .../x86_64-pc-linux-gnu/libstdc++-v3/include/complex:149:48: error: converting to ‘__bf16’ from ‘_Float64’ with greater conversion rank
> .../x86_64-pc-linux-gnu/libstdc++-v3/include/complex:149:27: error: converting to ‘__bf16’ from ‘_Float128’ with greater conversion rank
> .../x86_64-pc-linux-gnu/libstdc++-v3/include/complex:149:48: error: converting to ‘__bf16’ from ‘_Float128’ with greater conversion rank
> .../x86_64-pc-linux-gnu/libstdc++-v3/include/complex:149:27: error: converting to ‘__bf16’ from ‘_Float16’ with unordered conversion ranks
> .../x86_64-pc-linux-gnu/libstdc++-v3/include/complex:149:48: error: converting to ‘__bf16’ from ‘_Float16’ with unordered conversion ranks
>
> __z doesn't have _Tp type, but _Up, which is some other floating
Right.
> point type. For the conversion constructors where we have
> explicit(false) all is fine, we've tested in the requires that
> _Up is implicitly convertable to _Tp.
> But if explicit(true), it is fine without the explicit casts only
> for long double -> float, long double -> double and double -> float
> casts which are ok for implicit casts but not in narrowing conversions
> with non-constants.
> Otherwise it results in the above diagnostics.
That seems like a compiler bug.
I don't see any reason for val(x) to be different from val(T(x)) when
T is decltype(val). They are both direct-initialization of T from x.
>
> > I think the diff between C++23 and older standards would be smaller
> > done like this:
> >
> > /// Converting constructor.
> > template<typename _Up>
> > #if __cplusplus > 202002L
> > explicit(!requires(_Up __u) { _Tp{__u}; })
> > #endif
> > _GLIBCXX_CONSTEXPR complex(const complex<_Up>& __z)
> > : _M_real(__z.real()), _M_imag(__z.imag()) { }
>
> I could go for this with the _Tp() casts everywhere, or
> make also that line (perhaps except for { }) also conditional on
> #if __cplusplus > 202002L.
>
> > This also means the constructor body is always defined on the same
> > line, which avoids warnings from ld.gold's -Wodr-violations which IIRC
> > is based on simple heuristics like the line where the function is
> > defined.
> >
> > Otherwise this looks great. If the above alternative works, please use
> > that, but OK for trunk either way (once all the other patches it
> > depends on are in).
>
> All the builtins patches are in since today, and the
> "Small extended float support tweaks" patch is in too (it had a small
> testsuite conflict due to the
> https://gcc.gnu.org/pipermail/libstdc++/2022-October/054849.html
> patch still pending, but I've just resolved that conflict).
> I think this patch doesn't depend on anything anymore.
>
> OT, I also get on the same testcase with -Wsystem-headers -pedantic-errors
> .../x86_64-pc-linux-gnu/libstdc++-v3/include/cmath:47:2: error: #include_next is a GCC extension
> .../x86_64-pc-linux-gnu/libstdc++-v3/include/bits/std_abs.h:38:2: error: #include_next is a GCC extension
> .../x86_64-pc-linux-gnu/libstdc++-v3/include/numbers:223:1: error: non-standard suffix on floating constant [-Wpedantic]
> .../x86_64-pc-linux-gnu/libstdc++-v3/include/numbers:223:1: error: non-standard suffix on floating constant [-Wpedantic]
> .../x86_64-pc-linux-gnu/libstdc++-v3/include/numbers:223:1: error: non-standard suffix on floating constant [-Wpedantic]
> .../x86_64-pc-linux-gnu/libstdc++-v3/include/numbers:223:1: error: non-standard suffix on floating constant [-Wpedantic]
> .../x86_64-pc-linux-gnu/libstdc++-v3/include/numbers:223:1: error: non-standard suffix on floating constant [-Wpedantic]
> .../x86_64-pc-linux-gnu/libstdc++-v3/include/numbers:223:1: error: non-standard suffix on floating constant [-Wpedantic]
> .../x86_64-pc-linux-gnu/libstdc++-v3/include/numbers:223:1: error: non-standard suffix on floating constant [-Wpedantic]
> .../x86_64-pc-linux-gnu/libstdc++-v3/include/numbers:223:1: error: non-standard suffix on floating constant [-Wpedantic]
> .../x86_64-pc-linux-gnu/libstdc++-v3/include/numbers:223:1: error: non-standard suffix on floating constant [-Wpedantic]
> .../x86_64-pc-linux-gnu/libstdc++-v3/include/numbers:223:1: error: non-standard suffix on floating constant [-Wpedantic]
> .../x86_64-pc-linux-gnu/libstdc++-v3/include/numbers:223:1: error: non-standard suffix on floating constant [-Wpedantic]
> .../x86_64-pc-linux-gnu/libstdc++-v3/include/numbers:223:1: error: non-standard suffix on floating constant [-Wpedantic]
> .../x86_64-pc-linux-gnu/libstdc++-v3/include/numbers:223:1: error: non-standard suffix on floating constant [-Wpedantic]
> .../x86_64-pc-linux-gnu/libstdc++-v3/include/cstdlib:79:2: error: #include_next is a GCC extension
> .../libstdc++-v3/libsupc++/compare:844:45: error: ISO C++ does not support ‘__int128’ for ‘type name’ [-Wpedantic]
> .../x86_64-pc-linux-gnu/libstdc++-v3/include/cstddef:91:36: error: ISO C++ does not support ‘__int128’ for ‘type name’ [-Wpedantic]
> .../x86_64-pc-linux-gnu/libstdc++-v3/include/cstddef:93:45: error: ISO C++ does not support ‘__int128’ for ‘type name’ [-Wpedantic]
> For the numbers case (__float128 in there), wonder if we can wrap it in __extension__ or
> use __extension__ __float128 as the type (apparently e.g. type_traits uses
> __extension__ before the template keyword of the specializations), for the __int128 cases perhaps
> too, no idea about #include_next though.
Yeah, __extension__ works in some places, although I think I've found
places it doesn't help.
The #include_next one has no way to disable it, which is annoying.
prev parent reply other threads:[~2022-10-31 17:05 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-10-21 15:58 Jakub Jelinek
2022-10-31 10:26 ` Jonathan Wakely
2022-10-31 16:57 ` Jakub Jelinek
2022-10-31 17:04 ` Jonathan Wakely [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CACb0b4mkhTnWUPOU+BEh5f3TrB9zccwhE28V_g_pLdKzCG69ng@mail.gmail.com \
--to=jwakely@redhat.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=jakub@redhat.com \
--cc=libstdc++@gcc.gnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).