Avoid compiler warning when sscanf writes qboolean through %i

[smcv]

.../code/game/g_client.cpp:760:6: warning: format '%i' expects argument of type 'int*', but argument 26 has type 'qboolean*' [-Wformat=]

[xycaleth]

Perhaps there should be a qbool_cast kind of function/macro, which takes some type, and converts it to qfalse or qtrue? Instead of having the ternary operator for qfalse/qtrue everywhere.

[smcv]

Perhaps there should be a qbool_cast kind of function/macro

Fair point.

[smcv]

Perhaps there should be a qbool_cast kind of function/macro, which takes some type, and converts it to qfalse or qtrue? Instead of having the ternary operator for qfalse/qtrue everywhere

I started replacing (x ? qtrue : qfalse), (x ? qfalse : qtrue) and (qboolean) x with calls to a new qboolean qbool_cast(int) and (for C++ code) qboolean operator!(int). So far, I'm at around 500 lines of changes that are nearly mechanical enough to automate, but not quite (assuming we want to avoid obscuring the code with unnecessary levels of ()), and I'm still not finished.

I don't think that's a change that I can be confident to have got right every time, and it's going to be even more boring for you to review than it is for me to do, so catching my mistakes seems unlikely. I don't want to make that change without being absolutely sure that it's the direction you want OpenJK to go in.

I suspect OpenJK might actually be better off moving to ISO C++ bool for the C++ parts, and ISO C (stdbool.h) bool for the C parts of multiplayer, perhaps #define'ing qboolean, qtrue and qfalse to bool, true and false respectively. I think all reasonable versions of gcc and clang have C99 stdbool.h, so the only constraint on doing that is whether the OpenJK maintainers are OK with ceasing to support MSVC < 2013. Hopefully support for a 17 year old C standard is less controversial than it used to be, now that even MSVC has got round to supporting it?

[Razish]

I think VS2013 is/should be our minimum VS required. Didn't we remove the < 2013 project generators?

My concern over qboolean -> bool is where the size for qboolean must be 4 bytes: networked structure size/alignment, disk read/write, there has also been cases of non-boolean values stored in qboolean variables.
(I am also secretly okay with having the entire MP codebase compiled as C++, as I do in my mod)

EDIT: I did not mean to close this >_< hit the wrong button

[smcv]

Didn't we remove the < 2013 project generators?

There appear to be batch files for 2013 and 2015. I don't know whether anyone builds with older, and if so, whether you'd make any effort to support it.

My concern over qboolean -> bool is where the size for qboolean must be 4 bytes: networked structure size/alignment, disk read/write

Strictly speaking I don't think enums are actually required to be 4 bytes, although any sane compiler vendor will make them 4 bytes because otherwise everyone else whose code makes that assumption will refuse to use that compiler.

I would tend to assume that anything that reads qbooleans directly from files or network packets is going to have problems with endianness anyway, so all reads/writes involving qboolean should probably be reading a uint32_t (or Quake's more-portable equivalent if it has one) and doing the cast or ?: to/from qboolean at the same time it does the byteswap if necessary?

I can see that there'd be a problem with this approach where we pass pointers to structs across the game/cgame/ui DLL boundary, where we have to stick to the traditional ABI. Any booleans in those structs probably do need to stay qboolean.

One possible approach would be to replace qboolean with bool in non-ABI code gradually, with replacement with bool signifying "I have reviewed/audited this particular bit of code, and it isn't doing anything stupid with non-boolean booleans"; then the amount of qboolean would hopefully decline over time, leaving only the places where it's genuinely part of the ABI.

there has also been cases of non-boolean values stored in qboolean variables

As far as I can see, this is what the qboolean enum is meant to prevent: it forces you to use qboolean b = cond ? qtrue : qfalse because in C++ it isn't valid to assign an int to an enum, thus avoiding things like qboolean isJedi = (flags & FLAG_JEDI) (which is wrong, unless FLAG_JEDI happens to be numerically equal to 1). But then that was too annoying for the original developers of this codebase, so there are (qboolean) casts all over the place, which defeat the point of that trick with the enum :-(

If non-boolean values in allegedly boolean variables are necessary, the other potential route is to admit defeat and have typedef int qboolean, which is more or less what GLib does.

It's looking as though this isn't going to be fixed immediately, so would it be OK to merge my patch (which silences some warnings and at least doesn't make the situation worse), and work incrementally towards a solution?

[smcv]

Updated branch fixes one more instance of the same issue.