7
\$\begingroup\$

I've encountered a readability issue in C++ when comparing the length of a vector to a scalar. Commonly, I see solutions like this:

Vector<float> v{23., 42.};
if (v.lengthSquared() < 8 * 8) ...

While this works, it's not very readable. I believe a more intuitive approach would be:

if (v.length() < 8) ...

To achieve this, I've written a LengthProxy for my vector class:

#include <cmath>
#include <cstdlib>
#include <iostream>

template<typename T>
concept Sqrtable = requires(T a) {
    { std::sqrt(a) } -> std::convertible_to<T>;
};

template <class T> requires Sqrtable<T>
class LengthProxy {
    T lengthSquared;
    public:
    LengthProxy(T lengthSquared) : lengthSquared(lengthSquared) {}
    T operator*() const { return std::sqrt(lengthSquared); }
    operator T() const { return std::sqrt(lengthSquared); }
#if __cplusplus >= 202002L
    auto operator<=>(const LengthProxy<T>& other) const { return lengthSquared <=> other.lengthSquared; }
    auto operator<=>(const auto& other) const { return lengthSquared <=> other * other; }
#endif
    auto operator==(const LengthProxy<T>& other) const { return lengthSquared == other.lengthSquared; }
    auto operator==(const auto& other) const { return lengthSquared == other * other; }
    auto operator!=(const LengthProxy<T>& other) const { return lengthSquared != other.lengthSquared; }
    auto operator!=(const auto& other) const { return lengthSquared != other * other; }
    auto operator<=(const LengthProxy<T>& other) const { return lengthSquared <= other.lengthSquared; }
    auto operator<=(const auto& other) const { return lengthSquared <= other * other; }
    auto operator>=(const LengthProxy<T>& other) const { return lengthSquared >= other.lengthSquared; }
    auto operator>=(const auto& other) const { return lengthSquared >= other * other; }
    auto operator<(const LengthProxy<T>& other) const { return lengthSquared < other.lengthSquared; }
    auto operator<(const auto& other) const { return lengthSquared < other * other; }
    auto operator>(const LengthProxy<T>& other) const { return lengthSquared > other.lengthSquared; }
    auto operator>(const auto& other) const { return lengthSquared > other * other; }
};


template <class T>
struct Vector2 {
    T x, y;
    Vector2(T x, T y) : x(x), y(y) {}
    auto length() const { return LengthProxy<T>(x * x + y * y); }
};

int main(int argc, char** argv) {
    if (argc < 4) return 0;
    Vector2<double> p(atof(argv[1]), atof(argv[2]));
    auto l = p.length();
    std::cout << (l < atof(argv[3])) << std::endl;
}

This LengthProxy aims to make length comparisons more readable without sacrificing performance. I'm utilizing C++20 features like concepts and the spaceship operator. Here are my specific concerns and queries:

  1. Readability vs. Performance: Does the LengthProxy improve readability at the cost of significant performance overhead, especially considering the use of std::sqrt in comparisons?
  2. Code Complexity: Although LengthProxy makes the calling code more readable, it adds complexity. Is this an acceptable trade-off, or are there simpler alternatives?
  3. C++20 Features: My solution relies heavily on C++20 features. Are there compatibility issues I should be aware of, or ways to make this more backward-compatible while maintaining readability?
  4. Testing and Benchmarks: I haven't yet conducted extensive tests, particularly for edge cases or performance benchmarks. Are there specific scenarios or tests you would recommend?
  5. Alternative Approaches: Is there a better or more idiomatic way to achieve the same goal in C++?

Any insights, suggestions, or critiques would be greatly appreciated.

\$\endgroup\$

3 Answers 3

5
\$\begingroup\$

Bad concept

template<typename T>
concept Sqrtable = requires(T a) {
    { std::sqrt(a) } -> std::convertible_to<T>;
};

This is a bad use of concepts. Concepts are not for testing what a type can do, they are for testing what a type is. A concept like “square-root-able” is silly… what you want is a number concept.

(And, yes, I am aware that the standard concepts library has a ton of "-able" concepts, like std::default_initializable and std::copyable, which seem to violate my point. However, these are very low-level building blocks. And unless you are making other very low-level building blocks, like a copy() algorithm (like std::ranges::copy())—which, again, is what the standard library mostly does—you normally need more than very low-level building block concepts. In real world user code, rather than those low-level concepts, you would probably use something a bit higher level, like std::regular (at least).)

You can already see why a “square-root-able” concept is a bad idea, I hope. If you did constrain LengthProxy as “square-root-able”, then you would be unable to rewrite it using std::hypot() instead of std::sqrt(), as @TobySpeight suggests. Your class would be locked into a specific, potentially sub-par implementation, because a concept is part of the public API; you can’t change it without breaking everyone else’s code.

In a perfect world, the standard library math functions would already be properly constrained, so there would be a xstd::number concept, and std::sqrt() and std::hypot() would both be constrained to use it. Which means that your Vector2 and LengthProxy would naturally both be using xstd::number, and everything would just work. It would be magical.

What you could, and probably should, do is make a decent number concept. This is not trivial. I’ve been considering something like that for my own library, but haven’t settled on a design. I’ve toyed around with a mathematical hierarchy of “natural number”, “rational number”, “real number”, and “complex number”; I’ve mucked around with defining numbers based on the standard concepts (like std::floating_point) or traits (like std::is_arithmetic); or by operations (as in requires (T a, T b) { { a + b } -> std::convertible_to<T>; /* etc. */ }.

If you had a number concept, then even if it were not literally constrained to work with std::sqrt() (or std::hypot()), types that satisfy the concept should support being “square-rooted”, even if not necessarily by std::sqrt() or std::hypot() specifically. That’s because, internally and/or conceptually, that’s how those functions work. They probably do something conceptually (if not literally) like using Newton’s method to iteratively solve for the square root, which really just boils down to a bunch of multiplications, divisions, and additions. Thus, anything that supports multiplying, dividing, and adding is automatically “square-root-able”. This is how concepts are supposed to work. (In practice, to support any number type, you’d probably want to rewrite your function more like using std::sqrt; sqrt(lengthSquared);, and trust in the gods of ADL to find the right sqrt() function. But that’s just a quirk of C++.)

But yeah, if you want a simple and clear rule for concepts: Don’t simply look at a class or function’s implementation, pull out the operations used, and then constrain the class or function using a list of those operations. Think of concepts as conceptual, and ask, “conceptually speaking, what should a type that should work with this class or function be?” In your case, you’re considering the components of a mathematical vector, so, it should be a mathematical number (or something conceptually equivalent).

So yeah, tl;dr:

  • “-able” concepts are bad.
  • Constrain on what things are, not on what they do.
  • Make a decent “number” concept; that’s all you need.

Questions

Readability vs. Performance: Does the LengthProxy improve readability at the cost of significant performance overhead, especially considering the use of std::sqrt in comparisons?

I don’t see what the “significant performance overhead” is. Isn’t the whole point of the proxy class to avoid the square root that would have otherwise been inevitable?

If you never use the raw length—if you only do a comparison—then where is the extra overhead? If you do use the raw length, then you were doomed to pay for a square root regardless, so, again, where is the extra overhead?

Of course, everything depends how you use the length. If you just use it once, and don’t hold on to it or pass it around, then the proxy wins every time. But if you use the length value more than once, things get dicier.

auto const v_length = v.length();

// Comparison with a scalar:
//  - total natural cost: 1 sqrt
//  - total proxy cost  : 1 multiply
// Advantage: proxy.
if (v_length > 1.0)

// With a scalar range:
//  - total natural cost: 1 sqrt
//  - total proxy cost  : 2 multiplies
// Advantage: proxy.
if ((v_length >= min) and (v_length <= max))

// Doing some math:
//  - total natural cost: 1 sqrt
//  - total proxy cost  : 1 sqrt
// Advantage: neither.
auto const x = v_length - 1.0;

// Doing more complex math:
//  - total natural cost: 1 sqrt
//  - total proxy cost  : 3 sqrt
// Advantage: natural.
auto const y = std::min(std::acos(v_length), std::asin(v_length)) / v_length;

So here is the trade-off in readability and performance:

  1. If you know length() is expensive (which is the natural, default assumption), then you should:
    • do simple comparisons with length_squared()
    • do complex stuff with length()
  2. If you know length() is cheap, but returns a proxy, then you should:
    • do simple comparisons with length()
    • do complex stuff with auto len = static_cast<T>(length()); and use len

Your challenge is that case 1 is the “natural”, default case. Anyone going in blind to a new codebase will do this by default. You want to change the calculus, which is fine… but now you need to communicate that to users. You need to change the default behaviour. That is not impossible… but it is hard.

I am always very wary of people talking up “readability” when changing factors that are universally known and understood, and that’s what you are doing here. In my mind “readability” doesn’t just involve being aesthetically pleasing, it also involves communicating well. And changing a universally understood meaning of something is not good communication. EVERYONE writing code dealing with vectors knows that length() is expensive… which is why everyone does the length_squared() dance. You want to make that universal knowledge wrong, and make code that EVERYONE understands no longer work the way they understand. Is that “more readable”? I struggle to agree.

You may disagree, and that’s fine, but I would point out that the odds are stacked heavily against you, because the “natural” case is, naturally, not only very well understood, it is very well-supported. As @TobySpeight pointed out, std::hypot() exists to guard against out-of-range computations for vector lengths, and anyone who knows their salt knows to check for v * v being out-of-range before doing length_squared() > (v * v) if that’s a potential problem. But length() > v appears safe (assuming length() is written properly, with std::hypot()). You need to make sure users understand that it is not; you need to make them understand that their standard assumptions have been violated. Again, not impossible… but hard. Is it worth it for a minor aesthetic improvement?

Code Complexity: Although LengthProxy makes the calling code more readable, it adds complexity. Is this an acceptable trade-off, or are there simpler alternatives?

It doesn’t really add all that much complexity (once the proxy class is pared down a bit), and most of that complexity is hidden from users.

C++20 Features: My solution relies heavily on C++20 features. Are there compatibility issues I should be aware of, or ways to make this more backward-compatible while maintaining readability?

Decide whether you’re C++20 or not. If you’re in, then you’re in. It makes no sense to use concepts without any checks, but then to wrap operator<=> in a guard.

If you’re not committed to C++20, then you have to guard both the concepts and the spaceship operator (and any other C++20 features).

So either do this:

#include <cmath>
#include <compare>

// Need a number concept.
template <number T>
class LengthProxy
{
    T _length_squared;

public:
    constexpr explicit LengthProxy(T v) : _length_squared{std::move(v)} {}

    constexpr operator auto() const
    {
        using std::sqrt;
        return sqrt(_length_squared);
    }

    constexpr auto operator<=>(LengthProxy const&) const = default;

    constexpr auto operator<=>(T const& t) const
    {
        return _length_squared <=> (t * t);
    }
};

Or do this:

#ifdef __has_include
#   if __has_include(<version>)
#       include <version>
#   endif
#endif

#include <cmath>

#ifdef __cpp_lib_three_way_comparison
#   include <compare>
#endif // __cpp_lib_three_way_comparison

template <typename T>
#ifdef __cpp_concepts
    requires number<T>
#endif // __cpp_concepts
class LengthProxy
{
    T _length_squared;

public:
    constexpr explicit LengthProxy(T v) : _length_squared{std::move(v)} {}

    constexpr operator auto() const
    {
        using std::sqrt;
        return sqrt(_length_squared);
    }

#ifdef __cpp_impl_three_way_comparison
    constexpr auto operator<=>(LengthProxy const&) const = default;

    constexpr auto operator<=>(T const& t) const
    {
        return _length_squared <=> (t * t);
    }
#else
    friend constexpr auto operator==(LengthProxy const& a, LengthProxy const& b) { return a._length_squared == b._length_squared; }
    friend constexpr auto operator!=(LengthProxy const& a, LengthProxy const& b) { return a._length_squared != b._length_squared; }
    friend constexpr auto operator<(LengthProxy const& a, LengthProxy const& b) { return a._length_squared < b._length_squared; }
    friend constexpr auto operator>(LengthProxy const& a, LengthProxy const& b) { return a._length_squared > b._length_squared; }
    friend constexpr auto operator<=(LengthProxy const& a, LengthProxy const& b) { return a._length_squared <= b._length_squared; }
    friend constexpr auto operator>=(LengthProxy const& a, LengthProxy const& b) { return a._length_squared >= b._length_squared; }

    friend constexpr auto operator==(LengthProxy const& p, T const& t) { return p._length_squared == (t * t); }
    friend constexpr auto operator!=(LengthProxy const& p, T const& t) { return p._length_squared != (t * t); }
    friend constexpr auto operator<(LengthProxy const& p, T const& t) { return p._length_squared < (t * t); }
    friend constexpr auto operator>(LengthProxy const& p, T const& t) { return p._length_squared > (t * t); }
    friend constexpr auto operator<=(LengthProxy const& p, T const& t) { return p._length_squared <= (t * t); }
    friend constexpr auto operator>=(LengthProxy const& p, T const& t) { return p._length_squared >= (t * t); }

    friend constexpr auto operator==(T const& t, LengthProxy const& p) { return (t * t) == p._length_squared; }
    friend constexpr auto operator!=(T const& t, LengthProxy const& p) { return (t * t) != p._length_squared; }
    friend constexpr auto operator<(T const& t, LengthProxy const& p) { return (t * t) < p._length_squared; }
    friend constexpr auto operator>(T const& t, LengthProxy const& p) { return (t * t) > p._length_squared; }
    friend constexpr auto operator<=(T const& t, LengthProxy const& p) { return (t * t) <= p._length_squared; }
    friend constexpr auto operator>=(T const& t, LengthProxy const& p) { return (t * t) >= p._length_squared; }
#endif // __cpp_impl_three_way_comparison
};

Pick a lane.

It’s 2023, and all compilers worth mentioning support at least the subset of C++20 that you’re using. I would suggest not worrying about it, and going all in on C++20.

Testing and Benchmarks: I haven't yet conducted extensive tests, particularly for edge cases or performance benchmarks. Are there specific scenarios or tests you would recommend?

Not particularly.

If you want to specify behaviour in the cases of x where x * x or any of the other operations overflows or underflows, then of course you’d need to test that. But I’d just declare that UB, and leave dealing with it to safe number wrappers. That way, people who know it won’t be a problem (which is the vast majority of the time) pay nothing, and people who are concerned about it can pay the whatever costs they are willing to incur via their safe number wrappers.

Alternative Approaches: Is there a better or more idiomatic way to achieve the same goal in C++?

I mean, the more idiomatic way is literally the way you are trying to avoid (“length_squared() > (x * x)”. You are deliberately trying to violate the idiom. Is that “better”? 🤷🏼

Other, miscellaneous comments about code

You need to include <compare> if you’re defining the spaceship operator (because, under the hood, the auto is being defined to one of the ordering types defined in <compare>).

You should really use more vertical space in the LengthProxy class. Everything is kinda squished together.

The converting constructor should be explicit but—and here I disagree with @TobySpeight—the conversion operator to T should not be explicit. It would be silly for a proxy type for T to have an explicit conversion to T. That kinda defeats the purpose. (Of course, here you have the issue that your conversion to T is expensive… but that’s just part of the whole “violating the idiom” thing you’ve committed to. You might consider having an optional<T> data member for the un-squared length, that gets set iff the un-squared length is ever needed, so you only ever pay the cost of sqrt() one time max. That makes the proxy heavier, but not by all that much, and if you are trucking around a ton of these proxies, something else has gone horribly awry.)

I don’t see the point of operator*. Don’t add stuff to a public interface that you don’t actually need.

However, an accessor to get the squared length could be useful. If I know length() is returning a proxy object, but I could use the squared length for some reason, there is no sense in getting the un-squared length (via operator T) then squaring it. There is also no sense in calling length_squared() to recalculate the squared length when it’s already right there in the proxy object.

You might also consider adding some noexcepts in there, even if only conditionally, for stuff that shouldn’t fail… which is most stuff in the proxy class. The constructor could probably be noexcept(std::is_nothrow_move_constructible_v<T>) (and you should move construct lengthSquared). The comparisons should be noexcept((std::declval<T const>() <=> std::declval<T const>()) and (std::declval<T const&>() <=> std::declval<T const&>())), or similar. Even the square root should theoretically never fail, because the class is being initialized with a value that should be positive, and should never over- or underflow because of how it was calculated. You can really double-down on that assumption by making the constructor private, and making the proxy a friend of Vector2, so only Vector2 can initialize it with (what you assume) are valid values.

And, of course, constexpr all the things.

\$\endgroup\$
1
  • \$\begingroup\$ This is an amazing review ! \$\endgroup\$
    – nowox
    Commented Dec 16, 2023 at 21:41
5
\$\begingroup\$

I see a few things that may help you improve your code.

Make single argument constructors explicit

You probably don't want weird problems with type conversions if you use this code later. To avoid that, declare the constructor explicit. See C.46

Use only necessary includes

Unless I'm mistaken, I don't see anything that needs cstdlib, so you don't need to include that.

Consider mathematical overflow

The summed square of a vector of numbers has the possibility of overflow. It's not expressly handled here, but depending on the domain of numbers you're using, you might want to consider handling that. See https://en.cppreference.com/w/cpp/numeric/math/math_errhandling for some possible ways to do that.

Rethink how this is used

I can think of at least three other ways to do this, depending on what the real goal is.

If you are intending only to use it with something like a Vector2, then you might consider instead using std::complex<> instead and calling std::abs() on that wherever you want the magnitude of the vector.

A more general alternative would be to use an array and use std::tranform_reduce to create the length. Note that one nice thing about this is that if your vector has a large number of dimensions, you can get parallel execution very easily, just by explicitly setting the execution policy for std::transform_reduce.

std::array<double, 2> p{a, b};
auto l = std::sqrt(std::transform_reduce(
        p.begin(), p.end(), 0.0, std::plus{},
        [](auto x){ return x * x; }));

Or, if you can use C++23, it's slightly simpler using fold_left:

std::array<double, 2> p{a, b};
auto l = std::sqrt(std::ranges::fold_left(p, 0.0,
     [](double a, double x){ return a + x * x; }));
\$\endgroup\$
4
\$\begingroup\$

If we provide <=> operator, we should not be providing the other relational operators (that's pretty much the point of <=>).

All the relations have a risk of overflow when calculating other * other (and so does the construction of LengthProxy<T>(x * x + y * y)). I'd like to see some evidence that this is not a problem - perhaps in the form of additional guard code.

I would prefer the conversions to and from T to be explicit. Implicit conversion causes surprises enough, and much more so when combined with template classes.

I'd just stick with a plain auto length() const { return std::hypot(x, y); } rather than adding this complexity, unless backed by a strong justification.

\$\endgroup\$
2
  • \$\begingroup\$ Strong justification would be performances I think, especially on MCU where sqrt is an expensive operation. \$\endgroup\$
    – nowox
    Commented Dec 16, 2023 at 17:42
  • 2
    \$\begingroup\$ Indeed - if the code included a comment with relevant measurements, it would be much more likely to be accepted. \$\endgroup\$ Commented Dec 17, 2023 at 10:23

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.