Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Draft
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

marco-langer
Copy link
Contributor

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

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

@sdebionne
Copy link
Contributor

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.

@marco-langer marco-langer changed the title added select_most_precise<T1, T2>::type WIP: added select_most_precise<T1, T2>::type May 7, 2022
@mloskot
Copy link
Member

mloskot commented May 17, 2022

@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
Copy link
Contributor Author

marco-langer commented May 19, 2022

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
Copy link
Member

mloskot commented May 19, 2022

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
Copy link
Contributor Author

Am I getting it right?

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

@mloskot
Copy link
Member

mloskot commented Jun 6, 2022

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

@marco-langer marco-langer marked this pull request as draft June 26, 2022 10:23
@marco-langer marco-langer changed the title WIP: added select_most_precise<T1, T2>::type refactor: added select_most_precise<T1, T2>::type Jun 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants