std::complex::operator*= too restrictive with types it accepts

[Quuxplusone]

Bugzilla Link	PR19189
Status	CONFIRMED
Importance	P normal
Reported by	David Grellscheid (d.grellscheid+llvm@gmail.com)
Reported on	2014-03-19 10:08:45 -0700
Last modified on	2018-02-13 07:52:08 -0800
Version	unspecified
Hardware	All All
CC	llvm-bugs@lists.llvm.org, mclow.lists@gmail.com
Fixed by commit(s)
Attachments
Blocks
Blocked by	PR30589
See also

Changeset r187529 for <complex> has imposed additional restrictions on the
accepted types in operator*=. An example of the problematic change is below.

Where previously only the existence of

  complex<_Tp> operator*(complex<_Tp>, complex<_Xp>)

was necessary for the implementation of operator*=,
now a constructor _Tp(_Xp) is required.

Is this new constraint deliberately there?

An example situation where the new behaviour is problematic is for physical
units types that should never be directly constructed from doubles, but which
can be multiplied by doubles.

Index: complex
===================================================================
--- complex     (revision 187528)
+++ complex     (revision 187529)
@@ -309,12 +309,12 @@
         }
     template<class _Xp> _LIBCPP_INLINE_VISIBILITY complex& operator*=(const complex<_Xp>& __c)
         {
-            *this = *this * __c;
+            *this = *this * complex(__c.real(), __c.imag());
             return *this;
         }
     template<class _Xp> _LIBCPP_INLINE_VISIBILITY complex& operator/=(const complex<_Xp>& __c)
         {
-            *this = *this / __c;
+            *this = *this / complex(__c.real(), __c.imag());
             return *this;
         }
 };

[Quuxplusone]

I made that change.

The problem here (as I understand it) was that the only way we have to multiply
two complex numbers requires them to both be of the same type.

The call in complex is on line #587):
    template<class _Tp>
    complex<_Tp>
    operator*(const complex<_Tp>& __z, const complex<_Tp>& __w);

To deal with that, I need to create a temporary complex of the desired type.

Consider the following code:
    complex<int> i {1,1};
    complex<long double> ld{3,3};
    i *= ld;

This would not work with the pre-187529 code.

It's possible that a refactoring of the multiply code to be more like this:

    template<class _Tp>
    complex<_Tp>
    __complex_multiply(const complex<_Tp>& __z, const _Tp&real, const _Tp &imag);

(and then have operator* call that) would re-enable the behavior that you want.

But I'd have to do some benchmarking, etc to make sure that adding an
additional call doesn't impact performance too much.

[Quuxplusone]

Thanks for looking at this again.

You're right, operator* is only defined for two identical types.
In my example of physical units, I have to (and can) define my own

 template <int N>
 complex<Energy<N>> operator * ( complex<Energy<N>>, complex<double> )

However, I cannot define my own

 template <int N>
 complex<Energy<N>> & operator *= ( complex<Energy<N>> &, complex<double> )

because of ambiguous overload with std::complex::operator*=

[Quuxplusone]

(In reply to David Grellscheid from comment #2)
> Thanks for looking at this again.
>
> You're right, operator* is only defined for two identical types.
> In my example of physical units, I have to (and can) define my own
>
>  template <int N>
>  complex<Energy<N>> operator * ( complex<Energy<N>>, complex<double> )

I ran across this yesterday, and it reminded me of this bug:

[complex.numbers]/2:
     The effect of instantiating the template complex for any type other than float, double, or long double is unspecified.

[Quuxplusone]

This is related to #30589