1

Trying to build Seven Kingdoms: Ancient Adversaries from release 2.15.5 using MSYS2 on Win10 x64.

My invocation is ./configure --prefix=/dbg CPPFLAGS=-DDEBUG CFLAGS="-g -O0" && make (adapted from here, I have never before used autotools).

I get this compile error, which looks legit:

In file included from ../include/ALL.h:33,
                 from OCOLTBL.cpp:25:
OCOLTBL.cpp: In member function 'void ColorTable::generate_table(int, PalDesc&, RGBColor (*)(RGBColor, int, int))':
OCOLTBL.cpp:194:54: error: ordered comparison of pointer with integer zero ('BYTE*' {aka 'unsigned char*'} and 'int')
  194 |         err_when(absScale < 0 || palD.reserved_color < 0 || palD.pal == NULL);
      |                                  ~~~~~~~~~~~~~~~~~~~~^~~
../include/OERROR.h:52:32: note: in definition of macro 'err_when'
   52 |    #define err_when(cond)   if(cond) err.internal(NULL,__FILE__, __LINE__)
      |                                ^~~~

Indeed the left hand side of this comparison is defined as unsigned char *.

I must be too young, for a change, to recognise this expression as sensible: In my time a pointer has always either been equal to nullptr (effectively 0) as per [conv.ptr] and [expr.eq] or been presumably-valid (no way to know for sure before dereferencing). "Negative" pointers are not a thing.

How was ptr < 0 supposed to work in the past, if ever, and how was it defined the last time before becoming invalid?

Zsar
  • 443
  • 3
  • 14
  • Judging by the usage of `NULL` instead of `0` (or `nullptr`), the code either wasn't written by a proficient C++ programmer, or it was ported from C, or the code is just building C code as C++. So this might have come from C rather once being allowed in C++. Or perhaps `reserved_color` once upon a time wasn't a pointer. Or the original author has some misconceptions about uninitialized variables. Anyway, pointers have order, though its implementation might differ and should be using `std::less` for full portability, so the comparison itself is fine if not useful. – Some programmer dude Jul 19 '23 at 08:06
  • 1
    In the old days some programmers misused the most significant bit of a pointer to indicate some special state. When these pointers where (implicitly) converted to signed integers, the comparison ( < 0) was used to check if this bit was set. Of course this was never a good idea. But I dimly remember that in the 90s, at least some C++ compilers accepted code like this. But that was almost 30 years ago. – Christian Halaszovich Jul 19 '23 at 08:15

1 Answers1

2

This was valid until the resolution of CWG583.

0 is a null pointer constant, just like nullptr ([conv.ptr]/1).

Relational operators bring their operands to a common type before performing the comparison.

Under the old rules, (T*)p < 0 (or (T*)p < nullptr) meant (T*)p < (T*)0 ([expr.rel]/2):

Pointer conversions and qualification conversions are performed on pointer operands (or on a pointer operand and a null pointer constant, or on two null pointer constants, at least one of which is non-integral) to bring them to their composite pointer type. If one operand is a null pointer constant, the composite pointer type is std::nullptr_t if the other operand is also a null pointer constant or, if the other operand is a pointer, the type of the other operand.

[ Note: this implies that any pointer can be compared to a null pointer constant [...] — end note ]

The result of such a comparison is unspecified unless the other operand is also null:

  • If two pointers p and q of the same type [...] are both null, then p<=q and p>=q both yield true and p<q and p>q both yield false.
  • [...] if only one of them is null, the results of p<q, p>q, p<=q, and p>=q are unspecified.

After N3624, [expr.rel]/2 says:

If both operands are pointers, pointer conversions and qualification conversions are performed to bring them to their composite pointer type. After conversions, the operands shall have the same type.

A null pointer constant does not have pointer type, so the conversions aren't applied in (T*)p < 0, and the comparison is ill-formed because the operands are of different types.

duck
  • 1,455
  • 2
  • 8
  • For a complete answer, a short paragraph about the encountered idiom would be nice, e.g. invalid or not, _how would_, _how could_ a check for `ptr < 0` ever do something sensible? Did some breaking change happen in the toolchains of the day, for Boost to be able to patch _only the checks_ to `ptr != 0` without touching a single assignment? Or were these checks simply always nonsense? – Zsar Jul 19 '23 at 13:14
  • (I could imagine that: The fail state would never be the _desired_ path, so maybe checking `ptr < 0` had become an idiom through mere cargo culting, after being sensible only for some forgotten, _specific_ library. In any case, while it made its way into Boost, the idiom does not seem to be _especially_ widespread, e.g. I had previously never seen it, even though a few times I have handled pre-ISO-C++ myself.) – Zsar Jul 19 '23 at 13:16
  • (Or should I better split that part off into a separate question?) – Zsar Jul 19 '23 at 13:34
  • 1
    The Boost issue mentions `0 < p`, which is different from `p < 0`. On common platforms (where pointer comparison is just an (unsigned) comparison of address values, and null is `0x0`), the former is equivalent to `p != 0`, while the latter is always `false`. – duck Jul 19 '23 at 15:57
  • Ah! True, I misread that. Good enough for me, it's probably 100% nonsense in the 7kaa code, then. Going to accept after the customary delay. Going to update the question accordingly, the two samples have nothing in common in this case, obviously `ptr > 0` is not-nonsensical in a context, where it implies `ptr != 0`. – Zsar Jul 19 '23 at 16:20
  • 1
    The programmer almost certainly meant `*(palD.reserved_color) < 0` and the implicit conversion of `0` to a null pointer value hid the typo all this time. – Ben Voigt Jul 19 '23 at 17:31