9

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:

Community
  • 1
  • 1
Kurt Schwehr
  • 2,638
  • 3
  • 24
  • 41

2 Answers2

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.

Louis Langholtz
  • 2,913
  • 3
  • 17
  • 40
  • 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. – Toby Speight Mar 21 '17 at 15:47
  • @TobySpeight It is matter of style, isn't it. I'd prefer something like `namespace x { template constexpr bool is_uninitialized(T v){return v >= -(std::numeric_limits::max)() && v <= (std::numeric_limits::max)();} }` and then `x::is_uninitialized(MinX)` – mloskot Mar 21 '17 at 17:27
  • @mloskot Your check breaks with `is_uninitialized(static_cast(-128))`. And it should probably be called `is_initialized`. – Maxim Egorushkin Mar 21 '17 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 Mar 21 '17 at 18:21
  • 1
    I presume that AABB is "Axis-Aligned Bounding Box." – Kurt Schwehr Mar 22 '17 at 20:41
  • @KurtSchwehr Yep. Thank you for listing out the meaning of the acronym! – Louis Langholtz Mar 22 '17 at 20:58
  • Infinity () could be signed. Is information would cover both signs in one go. – emsr Mar 24 '17 at 04:01
  • What do you mean by "it's the most accurate approach"? IEEE 754 has exactly one representation of +Infinity and -Infinity. – ZachB Mar 22 '18 at 16:39
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.

jhill515
  • 894
  • 8
  • 26