boostorg / gil

Boost.GIL - Generic Image Library | Requires C++14 since Boost 1.80
https://boostorg.github.io/gil
Boost Software License 1.0
178 stars 163 forks source link

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

Open marco-langer opened 2 years ago

marco-langer commented 2 years ago

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:

Some pending questions:

Any thoughts on this?

Tasklist

sdebionne commented 2 years ago

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.

mloskot commented 2 years ago

@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);
marco-langer commented 2 years ago

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.

mloskot commented 2 years ago

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?

marco-langer commented 2 years ago

Am I getting it right?

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

mloskot commented 2 years ago

@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.