Re: [PATCH] libstdc++/complex: Remove implicit type casts in complex

[Jonathan Wakely <jwakely@redhat.com>]

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