AngusJohnson / Clipper2

Polygon Clipping and Offsetting - C++, C# and Delphi
Boost Software License 1.0
1.45k stars 266 forks source link

'round': ambiguous call to overloaded function #824

Closed schellingerhout closed 5 months ago

schellingerhout commented 5 months ago

Visual studio 2022, C++ 20, Warning level 4, using Clipper 2 via vcpkg. Clipper2 1.3.0

    template <typename T2>
    inline void Init(const T2 x_ = 0, const T2 y_ = 0)
    {
      if constexpr (std::numeric_limits<T>::is_integer &&
        !std::numeric_limits<T2>::is_integer) 
      {
        x = static_cast<T>(std::round(x_));  // <<<<<<<< C2668 'round': ambiguous call to overloaded function
        y = static_cast<T>(std::round(y_));
      }
      else
      {
        x = static_cast<T>(x_);
        y = static_cast<T>(y_);
      }
    }

I can remedy this simply by replacing !std::numeric_limits<T2>::is_integer with std::is_floating_point_v<T2>, alternative would be to specify an template explictly or to cast, I would rather use the is_floating_point primary type trait because those are simple, compiler optimized and mutually exclusive (each type is always and only in one primary type trait category).

I am not sure why numeric_limts was used this way (not for limits but type classing), I would recommend that primary type traits be used throughout instead including using is_integeral for integer types.

https://en.cppreference.com/w/cpp/types/is_floating_point https://en.cppreference.com/w/cpp/types/is_integral

Here is my proposed solution that would help if structured types are used that need to be rounded, but are not necessarily is_floating_point or is_integral_type because the primary types would match custom or extended types from boost most likely as is_class because they are wrappers.


// can we call std::round on the type? default false
template <typename T, typename = void>
struct is_round_invocable : std::false_type {};

template <typename T>
struct is_round_invocable<T, std::void_t<decltype(std::round(std::declval<T>()))>> : std::true_type {};

...

    if constexpr (is_round_invocable<T2>::value && !std::is_integral_v<T2>) 
    {
        x = static_cast<T>(std::round(x_));
        y = static_cast<T>(std::round(y_));
    } 
    else 
    {
        x = static_cast<T>(x_);
        y = static_cast<T>(y_);
    }
AngusJohnson commented 5 months ago

Thank you for the very helpful feedback, I've made the changes you've suggested and will upload them shortly.

I am not sure why numeric_limts was used this way

Just showing my inexperience with C++ 😱.

schellingerhout commented 5 months ago

Thank you for the very helpful feedback, I've made the changes you've suggested and will upload them shortly.

I am not sure why numeric_limts was used this way

Just showing my inexperience with C++ 😱.

There is actually very good reason to use std::numeric_limits<T>::is_integer in cases where a user defined type is used. Developers may define a template specialization on std::numeric_limits<T>::is_integer, because they are not allowed to specialize std::is_integral - or the other primary type traits. The problem is as soon as you create a user type it will detected as std::is_class, and you are not allowed to change that behavior, but the common solution is to fall back on std::numeric_limits<T>::is_integer.

None of what I typed above changes my proposed solution. It basically just rounds if round is supported and the type is not an internal integral type.

P.S. I've been using Clipper in Delphi for a very long time. I appreciate all your work and sharing that with the world

* It leads to undefined behavior.