gil icon indicating copy to clipboard operation
gil copied to clipboard

refactor: added select_most_precise<T1, T2>::type

Open marco-langer opened this issue 3 years ago • 6 comments

Description

This is a first draft to implement a compile-time selection of a most precise type as suggested in #363.

It follows closely the solution offered by Boost.Geometry, with the following changes:

  • arity of just 2 (but variable arity can be added later)
  • modernized to C++11
  • improved error handling in cases where one cannot select one of both types

Some pending questions:

  • std::is_floating_point does not support floating point types other than float, double and long double. Do we expect other floating point types?

Any thoughts on this?

Tasklist

  • [x] Add test case(s)
  • [ ] Ensure all CI builds pass
  • [ ] Review and approve

marco-langer avatar May 05 '22 07:05 marco-langer

Shall we use std::common_type that has the advantage of following the usual promotion rules and might be specialized by users to support more exotic types (fixed point, multi precision...)?

I found this MP11 example inspiring.

sdebionne avatar May 05 '22 08:05 sdebionne

@marco-langer Boost.Geometry's version allows pairs of integers

using B = boost::geometry::select_most_precise<short, short>::type;

while your variant requires one of the two must be float-point. Is that intentional?

@sdebionne

Shall we use std::common_type that has the advantage of following the usual promotion rules ... ?

Good question. It depends ~as~ if we want or expect this:

static_assert(std::is_same<decltype(short{} * short{}), int>::value);

instead of this:

using T = std::common_type<short, short>::type;
static_assert(std::is_same<T, short>::value);

or, BTW, instead of this:

using T = boost::geometry::select_most_precise<short, short>::type;
static_assert(std::is_same<UT, short>::value);

mloskot avatar May 17 '22 22:05 mloskot

Yes, disallowing two integral types was intentional (and I forgot to mention it in my above enumeration). While implementing my solution I too came to the same conclusion as @sdebionne, that the Boost.Geometry solution is problematic with respect to missing promotion rules.

While your example seems to be quite reasonable

using T = boost::geometry::select_most_precise<short, short>::type;
static_assert(std::is_same<T, short>::value);

consider an example in which both types have the same size but different signedness

  using T1 = boost::geometry::select_most_precise<short, unsigned short>::type;
  using T2 = boost::geometry::select_most_precise<unsigned short, short>::type;

  static_assert(std::is_same<T1, short>::value);
  static_assert(std::is_same<T2, unsigned short>::value);

This non-commutativity is either a.) just unintuitive or b.) a potential source of bugs. One can easily contrive examples in which there is a chance of signed integer overflow if calling this trait with the wrong order of template arguments.

marco-langer avatar May 19 '22 06:05 marco-langer

This non-commutativity is either a.) just unintuitive or b.) a potential source of bugs.

A very good point.

While implementing my solution I too came to the same conclusion as @sdebionne, that the Boost.Geometry solution is problematic with respect to missing promotion rules.

So, we are leaning towards the std::common_type and given the C++ rule

static_assert(std::is_same<decltype(short{} * short{}), int>::value);

we are agreeing on the following type selection:

static_assert
(
    std::is_same
    <
        std::common_type
        <
            decltype(short{} * short{}),
            short
        >::type,
        int
    >::value
);

Am I getting it right?

mloskot avatar May 19 '22 07:05 mloskot

Am I getting it right?

Maybe we should have a look at some concrete use cases of this trait before answering this question?

marco-langer avatar Jun 04 '22 03:06 marco-langer

@marco-langer How about @sdebionne 's #204 ? n.b. it's also a follow-up to #363 discussion, so could be fixed/closed by this PR too.

mloskot avatar Jun 06 '22 18:06 mloskot