Closed malespiaut closed 6 months ago
Thanks for this. But, why?
I mean, I could understand if you would want to get rid of type long
, because that's an ambigious type and has different widths on different platforms. But what is wrong about int
or short
? And apart from this, byte
is an established alias for unsigned char
, at least in the source port community.
However, my biggest headache is stdbool
. Are we sure how wide this type is? Is it 8-bite wide as unsigned char
that it's going to replace? Is this type used anywhere in file i/o or in structs where element size matters?
Also, it was nice being able to distinguish int
variables used as 16.16 fixed point numbers by consistently using the fixed
type.
Last, but not least, what is ZIG and what are its adavantages over plain Automake? If we are ever going to change the build system, that'd be CMake I guess.
Hello @fabiangreffrath,
I recognize that I may have gone overboard, and I would understand no problem if that PR gets rejected in the end.
I mean, I could understand if you would want to get rid of type long, because that's an ambigious type and has different widths on different platforms. But what is wrong about int or short? And apart from this, byte is an established alias for unsigned char, at least in the source port community.
That's only a matter of personal preference. I value explicit expressions, therefore I prefer to read int16_t
rather than short
.
Types are also inconsistent through the code: fixed
and int
, unsigned
and unsigned int
, short
and short int
, unsigned short
and unsigned short int
, char
and signed char
, etc.
So, as I said, it's a matter of personal preference, since I find the stdint
types straightforward to use and understand.
However, my biggest headache is
stdbool
. Are we sure how wide this type is? Is it 8-bite wide asunsigned char
that it's going to replace? Is this type used anywhere in file i/o or in structs where element size matters?
That's fair. I'll review my dedicated commit more thoroughly. But running the game shows no problem so far.
Also, it was nice being able to distinguish
int
variables used as 16.16 fixed point numbers by consistently using thefixed
type.
Since fixed
is defined as typedef int fixed;
in rt_def.h:269
, I would like to understand your point of view about not replacing fixed
with int
.
Last, but not least, what is ZIG and what are its adavantages over plain Automake? If we are ever going to change the build system, that'd be CMake I guess.
Zig is a low-level programming language that aims to fix the technical debt of C.
Zig can be used as a zero-dependency, drop-in C/C++ compiler that supports cross-compilation out-of-the-box.
Again, that's a matter of personal preference, but Zig have a built-in build system that is programmable in Zig, using a single build.zig
file.
The goal of having using the Zig build system:
Types are also inconsistent through the code:
fixed
andint
,unsigned
andunsigned int
,short
andshort int
,unsigned short
andunsigned short int
,char
andsigned char
, etc.
Yes, that's annoying. For some odd reasons I prefer unsigned int
over unsigned
, but at the same time short
over short int
. char
is a special case, though, as its implicit sign is indeed implementation specific and I'd prefer to have it explicitly given (or through an alias such as byte
).
That's fair. I'll review my dedicated commit more thoroughly. But running the game shows no problem so far.
That's good. It's important that e.g. loading levels, saving/restoring games and running demos still works as before.
Since
fixed
is defined astypedef int fixed;
inrt_def.h:269
, I would like to understand your point of view about not replacingfixed
withint
.
Although technically, both are 32-bit integers, semantically they aren't identical. A fixed
type number is actually a 16.16 fixed point number, e.g. an archaic but efficient way to express floating point calculus from the pre-hardware-float
era, and not meant to be actually interpreted as an integer.
- Have a simpler build system than Autotools or CMake (again, that's a personal preference: I've invested a lot of time with Autotools on different projects, and I really dislike it);
- In the long run, have a simple solution to cross-compile ROTT on many platforms.
Fine, alright, I'm going to accept this as an experiment. 😉 But it's supposed to be at least able to build SHAREWARE binaries and also accept the DATADIR macro.
In the long term, though, I don't think there's a way around CMake, since it not only makes it possible to cross-compile, but also provides a drop-in solution for MSVC which many people appreciate.
Yes, that's annoying. For some odd reasons I prefer
unsigned int
overunsigned
, but at the same timeshort
overshort int
.char
is a special case, though, as its implicit sign is indeed implementation specific and I'd prefer to have it explicitly given (or through an alias such asbyte
).
We agree on that issue. If stdint
types are to be rejected, we agree 100% about that.
Although I have to admit that I use both char
and int8_t
in my code, to make an explicit difference between strings and numerical values. But that's just me.
That's good. It's important that e.g. loading levels, saving/restoring games and running demos still works as before.
I think the easiest way to do it is to put a breakpoint on SafeRead()
and see which data structures are used in the call graph. I'll do that slowly in the coming days.
A “good” sign is that demos are running as incorrectly as before, hahaha.
Although technically, both are 32-bit integers, semantically they aren't identical. A
fixed
type number is actually a 16.16 fixed point number, e.g. an archaic but efficient way to express floating point calculus from the pre-hardware-float
era, and not meant to be actually interpreted as an integer.
Alright, I understand. But in that case, wouldn't it be better to define a more complex type than just typedef int fixed;
? Sounds like this should be defined as a union
of short[2]
(upper,lower) and int
.
Fine, alright, I'm going to accept this as an experiment. 😉 But it's supposed to be at least able to build SHAREWARE binaries and also accept the DATADIR macro.
It should do that fine. That's my responsibility after all! If it fails to do that, I won't have anything against dropping that idea.
In the long term, though, I don't think there's a way around CMake, since it not only makes it possible to cross-compile, but also provides a drop-in solution for MSVC which many people appreciate.
I understand that. Although given the recent development of Zig on Windows, I think that nothing is impossible before it hits v1.0.
But again, I'm no zealot. I'll just be following your decisions.
We agree on that issue. If
stdint
types are to be rejected, we agree 100% about that.
Fine!
Although I have to admit that I use both
char
andint8_t
in my code, to make an explicit difference between strings and numerical values. But that's just me.
Same here, though I use char
for strings and (try to be consistent with using) signed char
or unsigned char
(or an alias thereof) for numerical values. 😉
I think the easiest way to do it is to put a breakpoint on
SafeRead()
and see which data structures are used in the call graph. I'll do that slowly in the coming days.
I don't expect anything complicated if savegame and demo loading works as before. And we currently don't have network support anyway.
Alright, I understand. But in that case, wouldn't it be better to define a more complex type than just
typedef int fixed;
? Sounds like this should be defined as aunion
ofshort[2]
(upper,lower) andint
.
Na, let's not make it more complicated than it already is. 😉
But again, I'm no zealot. I'll just be following your decisions.
Now you made me curious about Zig as a build system. Maybe a rather simple project like this, with only few external dependencies and no embedded libraries and stuff, is a good starting point to experiment with it.
Hello @fabiangreffrath,
After much pondering about your concern about the width of bool
(which can vary) and reading Chris Wellons's article “My personal C coding style as of late 2023”, I came to the conclusion that it was probably wiser to keep defining our own boolean type to ensure its width is unchanged.
Therefore I've intoduced bool8_t
with TRUE
and FALSE
.
// In rt_def.h
typedef uint8_t bool8_t;
#define FALSE 0
#define TRUE 1
My reasonning for not naming these bool
, true
, and false
, is because I consider that these names are already taken by the standard, and shouldn't be redefined, for the sake of clarity.
It's done some testing, and everything seems to work. As a counter argument, setting bool8_t
to a wider type than uint8_t
makes save games incompatibles.
Let me know what you think.
Hm, but then it's yet another custom type. Currently rt_def.h
has this line:
typedef unsigned char boolean;
I don't see what's so wrong or ambigious about it that we need to give it a different name and while at it introduce entirely new and non-standard definitions of TRUE
and FALSE
. 🤷
I think that you are right. I'll clean up the mess and revert back the changes to what boolean
used to be.
Although it may not seem like it, this interesting discussion and reflection has been beneficial to my programming skills! 😃
Gets rid of all the
byte
,word
,longword
, andboolean
types, and replaces them with standardstdbool
andstdint
types. Throwing int a bonusbuild.zig
file that builds the binary with all the sanitizers.build.zig
is a no-brainer autotools alternative which supports cross-compiling out-of-the-box; compiles the C code with Zig, which is, for now, using libclang to do the job.