BobSteagall / wg21

Various documents and code related to proposals for WG21
University of Illinois/NCSA Open Source License
65 stars 11 forks source link

multiply operations on scalars should be constrained #58

Open kwikius opened 4 years ago

kwikius commented 4 years ago

re compatability with units library ( and indeed other libraries)

https://github.com/mpusz/units

Both units library and la library should constrain operators multiplying by scalars to avoid ambiguous overloads. LA library should define a scalar concept or use SFINAE version for legacy. Scalar is mentioned barely in the documentation but is an important concept for LA library

See example of the current situation and how to fix

https://godbolt.org/z/K-rWPL

( uncomment one of CONCEPT_CONSTRAINED_S, SFINAE_CONSTRAINED_S ) to get it to work

Below are ( some of) relevant functions

https://github.com/BobSteagall/wg21/blob/master/include/linear_algebra/arithmetic_operators.hpp#L104 https://github.com/BobSteagall/wg21/blob/master/include/linear_algebra/arithmetic_operators.hpp#L116 https://github.com/BobSteagall/wg21/blob/master/include/linear_algebra/arithmetic_operators.hpp#L130 https://github.com/BobSteagall/wg21/blob/master/include/linear_algebra/arithmetic_operators.hpp#L142

(Also incidentally applies to units library) ....

units uses concept but units::Scalar concept is basically Any but quantity. Not really constrained so ambiguous https://github.com/mpusz/units/blob/master/src/include/units/quantity.h#L325 https://github.com/mpusz/units/blob/master/src/include/units/concepts.h#L292

Currently standard library doesnt have unconstrained binary operators as far as I know

mhoemmen commented 4 years ago

std::is_arithmetic is probably more restrictive than one might like, considering that one can't add specializations. I'm guessing that was just a proxy for the actual concept you want, though. This reminds me a bit of P1813, which aims to introduce concepts for algorithms like std::reduce.

kwikius commented 4 years ago

std::is_arithmetic is probably more restrictive than one might like, considering that one can't add specializations. I'm guessing that was just a proxy for the actual concept you want, though. This reminds me a bit of P1813, which aims to introduce concepts for algorithms like std::reduce.

Absolutely std::is_arithmetic is too restrictive, but maybe is a good default. For customisation I think one can do\:

template <typename T> 
inline constexpr bool is_scalar_impl = std::is_arithmetic_v<T>;

template <typename T>
concept scalar = is_scalar_impl<T>; 

// ##########customisation via the constant  #############
template <typename T>
inline constexpr bool is_scalar_impl<my_quantity<T> > = true;
//##########################

P1813 is very interesting. There is a debate to be had about using "structural" ( quacks like a duck) concepts rather than opting your type in manually , as above.

mhoemmen commented 4 years ago

Btw I'm not entirely advocating P1813 as it stands. For instance, I don't think it's right to constrain std::reduce to types with associative operator+. Many users are OK with floating-point arithmetic (or other systems, e.g., saturating integers) being nonassociative. GENERALIZED_SUM exists to tell you what std::reduce does, so that you can evaluate whether to accept its results.