Skip to content

Mixed-signedness integer comparison is incorrect #703

@chiphogg

Description

@chiphogg

See https://godbolt.org/z/sn1eWTd4Y for example.

Of course, this problem is broadly known, which is why std::cmp_less and friends were added to C++20. As mp-units mostly just wraps the underlying raw types, we'd expect to have this problem here too. However, it's actually worse in this case, because the implementation of operator<=> explicitly casts to the common type, and this explicit cast will silence any possible warnings from -Wsign-compare.

OK, so we can be a little more careful in what we cast to before comparing, right? Well, even then we're not done, because it turns out that many (most?) compiler settings will suppress warnings that originate inside of library code. This means that even for situations where -1 < 1u will reliably raise a warning, (-1 * m) < (1u * m) will often not raise the warning!

Au just found and fixed this error (aurora-opensource/au#428), so we know exactly the steps that need to happen. We used 4 PRs, but not all of them would be relevant for mp-units. I think these are the important PRs/steps.

  1. Avoid changing signedness of integral rep aurora-opensource/au#433: be careful what rep you use for the LHS and RHS. If the common rep is std::integral, then use the common rep for each, except avoid changing the signedness.
  2. Fix comparisons for mixed-signedness integral reps aurora-opensource/au#434: if the common rep is integral, perform a careful comparison that takes this into account: kind of like "the operator<=> version of std::cmp_less". (mp-units' C++20 minimum version should enable you to do this more elegantly than we were able to.)

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions