Skip to content

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

Draft
marco-langer wants to merge 2 commits intoboostorg:developfrom
marco-langer:select_most_precise
Draft

refactor: added select_most_precise<T1, T2>::type#661
marco-langer wants to merge 2 commits intoboostorg:developfrom
marco-langer:select_most_precise

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

3 participants