10

This is likely bike-shedding, but maybe there is something interesting I'm missing...

If a class initializes a member val to std::numeric_limits<double>::infinity() and later wants to check if val has been changed to something valid (+/- inf are not valid here), what are the tradeoffs of these 3 approaches and did I miss any other interesting ways to solve this. (removed const for readability in the examples.)

bool IsInit() { return MinX != std::numeric_limits<double>::infinity(); }  // a

bool IsInit() { return !std::isinf(MinX); }  // b

bool IsInit() { return std::isfinite(MinX); }  // c

For now the code is C++03, but how would the options change with C++11, C++14 and C++17. e.g. with C++17, this code could be just std::optional<double> val. Or would quiet NaN be a safer bet just incase +/-inf became valid in the future?

This came up when I was reading this patch to this code:

For easy reference:

Sort of related:

0

2 Answers 2

4

Using these special values as a flag for initialization is only valid so long as the variable cannot otherwise get to the special value. Infinity isn't that hard to come by in terms of mathematical operations as overflow will also get it. So that could be problematic. Using quiet NaN if it's available would be easier to use if all that the invariant is, is that this floating point value can't be set by the user to this value.

As far as the three approaches are concerned, only approach "a" exactly matches the initial value. So it's the most accurate approach if the invariant is to be that only that initial value represents being the initialized value. Of course none of these approaches do anything with respect to protecting the invariant to begin with, which is more the issue in my mind: whether or not the invariant is or can be effectively enforced.

While this isn't a code review site per-say, in terms of the linked code edit to the OGREnvelope class (which appears to just be an AABB) on github, I think a few things should be noted:

  • The IsInit - based on its code - appears meant to be returning whether the AABB has actually been set by the user. Not whether whether it's in its initial state/value as arguably IsInit suggests (and misled me to believe). Personally I prefer a name like IsValid for this test.
  • As to what the invariant actually is, the edit itself seems to recognize that a valid AABB should never have a MinX value that's greater than MaxX nor a MinY that's greater than MaxY. In other words, they should never be reversed in a valid AABB. MinX should always be less than or equal to MaxX (and same for min and max Y variables).
  • Using reversed infinity values as is done, makes it: (a) easier to potentially grow the AABB to envelop other AABBs (and the edit demonstrates what I mean by that in reducing the lines of code from 14 to 4); and (b) makes it capable of handling a wider range of valid AABBs than using the max() and lowest() values would.
  • The fact that the MinX, MaxX, MinY, MaxY member variables are publicly accessible means that IsInit and the invariant itself is only advisory in the sense that there's no encapsulation to protect the invariant.

In the broader context of these things being noted, NaN isn't appropriate. The invariant was never really that MinX != std::numeric_limits<double>::infinity(). And IsInit (as implemented in that code) is at best incomplete. In the context of it being renamed to something like IsValid, then a more logically consistent implementation is:

bool IsValid() const
{
    return !std::isnan(MinX) && !std::isnan(MinY) && MinX <= MaxX && MinY <= MaxY;
}

With this implementation, a valid AABB is an OGREnvelope whose values are all valid numbers (none of which are NaN) and whose min values must be less than or equal to the max values for X and Y respectively.

8
  • 2
    The other advantage of option 'a' is that you can give the constant a meaningful name in your class (as a private static constant), so that the MinX != UNINITIALISED_VAL test is clearer. Commented Mar 21, 2017 at 15:47
  • @TobySpeight It is matter of style, isn't it. I'd prefer something like namespace x { template <typename T> constexpr bool is_uninitialized(T v){return v >= -(std::numeric_limits<T>::max)() && v <= (std::numeric_limits<T>::max)();} } and then x::is_uninitialized(MinX)
    – mloskot
    Commented Mar 21, 2017 at 17:27
  • @mloskot Your check breaks with is_uninitialized(static_cast<signed char>(-128)). And it should probably be called is_initialized. Commented Mar 21, 2017 at 18:17
  • @MaximEgorushkin Yes, it does break. It's just an illustration of alternative to named constant. It is not a complete and mathematically valid implementation for all possible numeric T, of course. Thanks for the name fix indeed (UNINITIALISED_VAL constant name copy&paste error, have I mentioned it is counter-example to the use of constant? :-))
    – mloskot
    Commented Mar 21, 2017 at 18:21
  • 1
    I presume that AABB is "Axis-Aligned Bounding Box." Commented Mar 22, 2017 at 20:41
0

I don't expect this to be an accepted answer, but it hopefully does offer some good information.

I did an outright comparison of the three proposed implementations above using the Godbolt tool: https://godbolt.org/z/184Zb6

The basic observation is that that with compiler optimizations turned on, comparing with std::numeric_limits<double>::infinity() is more efficient (fewer CPU instructions and memory moves) than std::isfinite(); that said, it's only more efficient than !std::isinf() because of the added logical negation operation.

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.