diff mbox series

libstdc++/complex: Remove implicit type casts in complex

Message ID CAG0zEH+0J741JS9PGKTJbucuijprZGhCFt9yJnuZd5aHk7SeBw@mail.gmail.com
State Changes Requested
Delegated to: Jonathan Wakely
Headers
Series libstdc++/complex: Remove implicit type casts in complex |

Commit Message

Weslley da Silva Pereira March 27, 2023, 9:23 p.m. UTC 
  Dear all,

Here follows a patch that removes implicit type casts in std::complex.

*Description:* The current implementation of `complex<_Tp>` assumes that
`int, double, long double` are explicitly convertible to `_Tp`. Moreover,
it also assumes that:

1. `int` is implicitly convertible to `_Tp`, e.g., when using
`complex<_Tp>(1)`.
2. `long double` can be attributed to a `_Tp` variable, e.g., when using
`const _Tp __pi_2 = 1.5707963267948966192313216916397514L`.

This patch transforms the implicit casts (1) and (2) into explicit type
casts. As a result, `std::complex` is now able to support more types. One
example is the type `Eigen::Half` from
https://eigen.tuxfamily.org/dox-devel/Half_8h_source.html which does not
implement implicit type conversions.

*ChangeLog:*
libstdc++-v3/ChangeLog:

        * include/std/complex:

*Patch:* fix_complex.diff. (Also at
https://github.com/gcc-mirror/gcc/pull/84)

*OBS:* I didn't find a good reason for adding new tests or test results
here since this is really a small upgrade (in my view) to std::complex.

Sincerely,
  Weslley

Comments

Jonathan Wakely May 11, 2023, 9:57 p.m. UTC | #1 
On Mon, 27 Mar 2023 at 22:25, Weslley da Silva Pereira via Libstdc++ <
libstdc++@gcc.gnu.org> wrote:

> Dear all,
>
> Here follows a patch that removes implicit type casts in std::complex.
>
> *Description:* The current implementation of `complex<_Tp>` assumes that
> `int, double, long double` are explicitly convertible to `_Tp`. Moreover,
> it also assumes that:
>
> 1. `int` is implicitly convertible to `_Tp`, e.g., when using
> `complex<_Tp>(1)`.
> 2. `long double` can be attributed to a `_Tp` variable, e.g., when using
> `const _Tp __pi_2 = 1.5707963267948966192313216916397514L`.
>
> This patch transforms the implicit casts (1) and (2) into explicit type
> casts. As a result, `std::complex` is now able to support more types. One
> example is the type `Eigen::Half` from
> https://eigen.tuxfamily.org/dox-devel/Half_8h_source.html which does not
> implement implicit type conversions.
>
> *ChangeLog:*
> libstdc++-v3/ChangeLog:
>
>         * include/std/complex:
>

Thank you for the patch. Now that we're in developement stage 1 for GCC 14,
it's time to consider it.

You're missing a proper changelog entry, I suggest:

       * include/std/complex (polar, __complex_sqrt)
       (__complex_pow_unsigned, pow, __complex_acos): Replace implicit
       conversions from int and long double to value_type.

You're also missing either a copyright assignment on file with the FSF
(unless you've completed that paperwork?), or a DCO sign-off. Please see
https://gcc.gnu.org/contribute.html#legal and https://gcc.gnu.org/dco.html
for more details.


>
> *Patch:* fix_complex.diff. (Also at
> https://github.com/gcc-mirror/gcc/pull/84)
>
> *OBS:* I didn't find a good reason for adding new tests or test results
> here since this is really a small upgrade (in my view) to std::complex.
>

I don't agree. The purpose of this is to support std::complex<Foo> for a
type Foo without implicit conversions (which isn't required by the standard
btw, only the floating-point types are required to work, but we can support
others as an extension). Without tests, we don't know if that goal has been
met, and we don't know if the goal continues to be met in future versions.
A test would ensure that we don't accidentally re-introduce code requiring
implicit conversions.

With a suitable test, I think this patch will be OK for GCC 14.

Thanks again for contributing.
Weslley da Silva Pereira Nov. 3, 2023, 5:47 p.m. UTC | #2 
Hi Jonathan,

I am sorry for the delay. The mailing lists libstdc++@gcc.gnu.org and
gcc-patches@gcc.gnu.org have just too many emails, so your email got lost.
I hope my changes still make sense to be included in GCC. Please, find my
comments below.

On Thu, May 11, 2023 at 3:57 PM Jonathan Wakely <jwakely@redhat.com> wrote:

>
>
> On Mon, 27 Mar 2023 at 22:25, Weslley da Silva Pereira via Libstdc++ <
> libstdc++@gcc.gnu.org> wrote:
>
>> Dear all,
>>
>> Here follows a patch that removes implicit type casts in std::complex.
>>
>> *Description:* The current implementation of `complex<_Tp>` assumes that
>> `int, double, long double` are explicitly convertible to `_Tp`. Moreover,
>> it also assumes that:
>>
>> 1. `int` is implicitly convertible to `_Tp`, e.g., when using
>> `complex<_Tp>(1)`.
>> 2. `long double` can be attributed to a `_Tp` variable, e.g., when using
>> `const _Tp __pi_2 = 1.5707963267948966192313216916397514L`.
>>
>> This patch transforms the implicit casts (1) and (2) into explicit type
>> casts. As a result, `std::complex` is now able to support more types. One
>> example is the type `Eigen::Half` from
>> https://eigen.tuxfamily.org/dox-devel/Half_8h_source.html which does not
>> implement implicit type conversions.
>>
>> *ChangeLog:*
>> libstdc++-v3/ChangeLog:
>>
>>         * include/std/complex:
>>
>
> Thank you for the patch. Now that we're in developement stage 1 for GCC
> 14, it's time to consider it.
>
> You're missing a proper changelog entry, I suggest:
>
>        * include/std/complex (polar, __complex_sqrt)
>        (__complex_pow_unsigned, pow, __complex_acos): Replace implicit
>        conversions from int and long double to value_type.
>

I agree with your proposal for the changelog.


> You're also missing either a copyright assignment on file with the FSF
> (unless you've completed that paperwork?), or a DCO sign-off. Please see
> https://gcc.gnu.org/contribute.html#legal and https://gcc.gnu.org/dco.html
> for more details.
>

Here is my DCO sign-off:

*Copyright:*
Signed-off-by: Weslley da Silva Pereira <weslley.spereira@gmail.com>


>
>
>>
>> *Patch:* fix_complex.diff. (Also at
>> https://github.com/gcc-mirror/gcc/pull/84)
>>
>> *OBS:* I didn't find a good reason for adding new tests or test results
>> here since this is really a small upgrade (in my view) to std::complex.
>>
>
> I don't agree. The purpose of this is to support std::complex<Foo> for a
> type Foo without implicit conversions (which isn't required by the standard
> btw, only the floating-point types are required to work, but we can support
> others as an extension). Without tests, we don't know if that goal has been
> met, and we don't know if the goal continues to be met in future versions.
> A test would ensure that we don't accidentally re-introduce code requiring
> implicit conversions.
>
> With a suitable test, I think this patch will be OK for GCC 14.
>
> Thanks again for contributing.
>
>
>
*Tests:*
See the attached file `test_complex_eigenhalf.cpp`

*Test results:*
- When using x86-64 GCC (trunk), I obtained compilation errors as shown in
the attached text file. Live example at: https://godbolt.org/z/oa9M34h8P.
- I observed no errors after applying the suggested patch on my machine.
- I tried it with the flag `-Wall`. No warnings were shown.
- My machine configuration and my GCC build information are displayed in
the file `config.log` generated by the configuration step of GCC.

Let me know if I need to do anything else.

Thanks,
  Weslley
Jonathan Wakely Nov. 6, 2023, 10:44 a.m. UTC | #3 
On Fri, 3 Nov 2023 at 17:47, Weslley da Silva Pereira
<weslley.spereira@gmail.com> wrote:
>
> Hi Jonathan,
>
> I am sorry for the delay. The mailing lists libstdc++@gcc.gnu.org and gcc-patches@gcc.gnu.org have just too many emails, so your email got lost. I hope my changes still make sense to be included in GCC. Please, find my comments below.

Hi,

Thanks for the updated patch, test etc. Yes, I think this still makes
sense and I'll take care of committing it.



>
> On Thu, May 11, 2023 at 3:57 PM Jonathan Wakely <jwakely@redhat.com> wrote:
>>
>>
>>
>> On Mon, 27 Mar 2023 at 22:25, Weslley da Silva Pereira via Libstdc++ <libstdc++@gcc.gnu.org> wrote:
>>>
>>> Dear all,
>>>
>>> Here follows a patch that removes implicit type casts in std::complex.
>>>
>>> *Description:* The current implementation of `complex<_Tp>` assumes that
>>> `int, double, long double` are explicitly convertible to `_Tp`. Moreover,
>>> it also assumes that:
>>>
>>> 1. `int` is implicitly convertible to `_Tp`, e.g., when using
>>> `complex<_Tp>(1)`.
>>> 2. `long double` can be attributed to a `_Tp` variable, e.g., when using
>>> `const _Tp __pi_2 = 1.5707963267948966192313216916397514L`.
>>>
>>> This patch transforms the implicit casts (1) and (2) into explicit type
>>> casts. As a result, `std::complex` is now able to support more types. One
>>> example is the type `Eigen::Half` from
>>> https://eigen.tuxfamily.org/dox-devel/Half_8h_source.html which does not
>>> implement implicit type conversions.
>>>
>>> *ChangeLog:*
>>> libstdc++-v3/ChangeLog:
>>>
>>>         * include/std/complex:
>>
>>
>> Thank you for the patch. Now that we're in developement stage 1 for GCC 14, it's time to consider it.
>>
>> You're missing a proper changelog entry, I suggest:
>>
>>        * include/std/complex (polar, __complex_sqrt)
>>        (__complex_pow_unsigned, pow, __complex_acos): Replace implicit
>>        conversions from int and long double to value_type.
>
>
> I agree with your proposal for the changelog.
>
>>
>> You're also missing either a copyright assignment on file with the FSF (unless you've completed that paperwork?), or a DCO sign-off. Please see https://gcc.gnu.org/contribute.html#legal and https://gcc.gnu.org/dco.html for more details.
>
>
> Here is my DCO sign-off:
>
> Copyright:
> Signed-off-by: Weslley da Silva Pereira <weslley.spereira@gmail.com>
>
>>
>>
>>>
>>>
>>> *Patch:* fix_complex.diff. (Also at
>>> https://github.com/gcc-mirror/gcc/pull/84)
>>>
>>> *OBS:* I didn't find a good reason for adding new tests or test results
>>> here since this is really a small upgrade (in my view) to std::complex.
>>
>>
>> I don't agree. The purpose of this is to support std::complex<Foo> for a type Foo without implicit conversions (which isn't required by the standard btw, only the floating-point types are required to work, but we can support others as an extension). Without tests, we don't know if that goal has been met, and we don't know if the goal continues to be met in future versions. A test would ensure that we don't accidentally re-introduce code requiring implicit conversions.
>>
>> With a suitable test, I think this patch will be OK for GCC 14.
>>
>> Thanks again for contributing.
>>
>>
>
> Tests:
> See the attached file `test_complex_eigenhalf.cpp`
>
> Test results:
> - When using x86-64 GCC (trunk), I obtained compilation errors as shown in the attached text file. Live example at: https://godbolt.org/z/oa9M34h8P.
> - I observed no errors after applying the suggested patch on my machine.
> - I tried it with the flag `-Wall`. No warnings were shown.
> - My machine configuration and my GCC build information are displayed in the file `config.log` generated by the configuration step of GCC.
>
> Let me know if I need to do anything else.
>
> Thanks,
>   Weslley
>
> --
> Weslley S. Pereira
Weslley da Silva Pereira Nov. 26, 2023, 1:49 a.m. UTC | #4 
Hi Jonathan,

Is there a way I can see my patch merged (when it gets merged)?
Particularly, I want to have a link for the commit. I would like to add
this as "impact on third party software" for the software
https://github.com/tlapack/tlapack.

Thanks,
  Weslley

On Mon, Nov 6, 2023 at 3:44 AM Jonathan Wakely <jwakely@redhat.com> wrote:

> On Fri, 3 Nov 2023 at 17:47, Weslley da Silva Pereira
> <weslley.spereira@gmail.com> wrote:
> >
> > Hi Jonathan,
> >
> > I am sorry for the delay. The mailing lists libstdc++@gcc.gnu.org and
> gcc-patches@gcc.gnu.org have just too many emails, so your email got
> lost. I hope my changes still make sense to be included in GCC. Please,
> find my comments below.
>
> Hi,
>
> Thanks for the updated patch, test etc. Yes, I think this still makes
> sense and I'll take care of committing it.
>
>
>
> >
> > On Thu, May 11, 2023 at 3:57 PM Jonathan Wakely <jwakely@redhat.com>
> wrote:
> >>
> >>
> >>
> >> On Mon, 27 Mar 2023 at 22:25, Weslley da Silva Pereira via Libstdc++ <
> libstdc++@gcc.gnu.org> wrote:
> >>>
> >>> Dear all,
> >>>
> >>> Here follows a patch that removes implicit type casts in std::complex.
> >>>
> >>> *Description:* The current implementation of `complex<_Tp>` assumes
> that
> >>> `int, double, long double` are explicitly convertible to `_Tp`.
> Moreover,
> >>> it also assumes that:
> >>>
> >>> 1. `int` is implicitly convertible to `_Tp`, e.g., when using
> >>> `complex<_Tp>(1)`.
> >>> 2. `long double` can be attributed to a `_Tp` variable, e.g., when
> using
> >>> `const _Tp __pi_2 = 1.5707963267948966192313216916397514L`.
> >>>
> >>> This patch transforms the implicit casts (1) and (2) into explicit type
> >>> casts. As a result, `std::complex` is now able to support more types.
> One
> >>> example is the type `Eigen::Half` from
> >>> https://eigen.tuxfamily.org/dox-devel/Half_8h_source.html which does
> not
> >>> implement implicit type conversions.
> >>>
> >>> *ChangeLog:*
> >>> libstdc++-v3/ChangeLog:
> >>>
> >>>         * include/std/complex:
> >>
> >>
> >> Thank you for the patch. Now that we're in developement stage 1 for GCC
> 14, it's time to consider it.
> >>
> >> You're missing a proper changelog entry, I suggest:
> >>
> >>        * include/std/complex (polar, __complex_sqrt)
> >>        (__complex_pow_unsigned, pow, __complex_acos): Replace implicit
> >>        conversions from int and long double to value_type.
> >
> >
> > I agree with your proposal for the changelog.
> >
> >>
> >> You're also missing either a copyright assignment on file with the FSF
> (unless you've completed that paperwork?), or a DCO sign-off. Please see
> https://gcc.gnu.org/contribute.html#legal and https://gcc.gnu.org/dco.html
> for more details.
> >
> >
> > Here is my DCO sign-off:
> >
> > Copyright:
> > Signed-off-by: Weslley da Silva Pereira <weslley.spereira@gmail.com>
> >
> >>
> >>
> >>>
> >>>
> >>> *Patch:* fix_complex.diff. (Also at
> >>> https://github.com/gcc-mirror/gcc/pull/84)
> >>>
> >>> *OBS:* I didn't find a good reason for adding new tests or test results
> >>> here since this is really a small upgrade (in my view) to std::complex.
> >>
> >>
> >> I don't agree. The purpose of this is to support std::complex<Foo> for
> a type Foo without implicit conversions (which isn't required by the
> standard btw, only the floating-point types are required to work, but we
> can support others as an extension). Without tests, we don't know if that
> goal has been met, and we don't know if the goal continues to be met in
> future versions. A test would ensure that we don't accidentally
> re-introduce code requiring implicit conversions.
> >>
> >> With a suitable test, I think this patch will be OK for GCC 14.
> >>
> >> Thanks again for contributing.
> >>
> >>
> >
> > Tests:
> > See the attached file `test_complex_eigenhalf.cpp`
> >
> > Test results:
> > - When using x86-64 GCC (trunk), I obtained compilation errors as shown
> in the attached text file. Live example at:
> https://godbolt.org/z/oa9M34h8P.
> > - I observed no errors after applying the suggested patch on my machine.
> > - I tried it with the flag `-Wall`. No warnings were shown.
> > - My machine configuration and my GCC build information are displayed in
> the file `config.log` generated by the configuration step of GCC.
> >
> > Let me know if I need to do anything else.
> >
> > Thanks,
> >   Weslley
> >
> > --
> > Weslley S. Pereira
>
>
diff mbox series

Patch

diff --git a/libstdc++-v3/include/std/complex b/libstdc++-v3/include/std/complex
index 0f5f14c3ddb..1a4ac8a2a54 100644
--- a/libstdc++-v3/include/std/complex
+++ b/libstdc++-v3/include/std/complex
@@ -80,7 +80,7 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
   template<typename _Tp>
     _GLIBCXX20_CONSTEXPR complex<_Tp> conj(const complex<_Tp>&);
   ///  Return complex with magnitude @a rho and angle @a theta.
-  template<typename _Tp> complex<_Tp> polar(const _Tp&, const _Tp& = 0);
+  template<typename _Tp> complex<_Tp> polar(const _Tp&, const _Tp& = _Tp(0));
 
   // Transcendentals:
   /// Return complex cosine of @a z.
@@ -961,7 +961,7 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
     inline complex<_Tp>
     polar(const _Tp& __rho, const _Tp& __theta)
     {
-      __glibcxx_assert( __rho >= 0 );
+      __glibcxx_assert( __rho >= _Tp(0) );
       return complex<_Tp>(__rho * cos(__theta), __rho * sin(__theta));
     }
 
@@ -1161,13 +1161,13 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
 
       if (__x == _Tp())
         {
-          _Tp __t = sqrt(abs(__y) / 2);
+          _Tp __t = sqrt(abs(__y) / _Tp(2));
           return complex<_Tp>(__t, __y < _Tp() ? -__t : __t);
         }
       else
         {
-          _Tp __t = sqrt(2 * (std::abs(__z) + abs(__x)));
-          _Tp __u = __t / 2;
+          _Tp __t = sqrt(_Tp(2) * (std::abs(__z) + abs(__x)));
+          _Tp __u = __t / _Tp(2);
           return __x > _Tp()
             ? complex<_Tp>(__u, __y / __t)
             : complex<_Tp>(abs(__y) / __t, __y < _Tp() ? -__u : __u);
@@ -1257,7 +1257,7 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
     complex<_Tp>
     __complex_pow_unsigned(complex<_Tp> __x, unsigned __n)
     {
-      complex<_Tp> __y = __n % 2 ? __x : complex<_Tp>(1);
+      complex<_Tp> __y = __n % 2 ? __x : complex<_Tp>(_Tp(1));
 
       while (__n >>= 1)
         {
@@ -1280,7 +1280,7 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
     pow(const complex<_Tp>& __z, int __n)
     {
       return __n < 0
-	? complex<_Tp>(1) / std::__complex_pow_unsigned(__z, -(unsigned)__n)
+	? complex<_Tp>(_Tp(1)) / std::__complex_pow_unsigned(__z, -(unsigned)__n)
         : std::__complex_pow_unsigned(__z, __n);
     }
 
@@ -2017,7 +2017,7 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
     __complex_acos(const std::complex<_Tp>& __z)
     {
       const std::complex<_Tp> __t = std::asin(__z);
-      const _Tp __pi_2 = 1.5707963267948966192313216916397514L;
+      const _Tp __pi_2 = _Tp(1.5707963267948966192313216916397514L);
       return std::complex<_Tp>(__pi_2 - __t.real(), -__t.imag());
     }