Interrupt / systemshock

Shockolate - A minimalist and cross platform System Shock source port.
GNU General Public License v3.0
812 stars 65 forks source link

Clean build #359

Closed shamazmazum closed 4 years ago

shamazmazum commented 4 years ago

Hi! These changes produce much much cleaner build when building with clang. My clang version is:

FreeBSD clang version 8.0.1 (tags/RELEASE_801/final 366581) (based on LLVM 8.0.1)
Target: x86_64-unknown-freebsd12.1
Thread model: posix
InstalledDir: /usr/bin

Some changes are purely cosmetic, like convert if (a = get_a()) {do_something_with (a);} to if ((a = get_a())) {do_something_with (a);}.

Other changes can possibly result in some functional differences, like conversion from if (flag && FOO) {} to if (flag & FOO) {}. One would like to keep the code "as is" not to break anything, but I think it's better to rewrite that to what the author's original intention possibly was.

Also, lots of UB fixed.

Finally, some warnings were not fixed, like taking absolute value of a variable of unsigned type. It's hard to say now, what the author was thinking of, so I just leave it as is.

Testing it now, already reached engineering level without problems ;-)

Interrupt commented 4 years ago

What's the benefit in wrapping some of these expressions in an extra layer of parentheses when branching? I haven't seen that style before.

shamazmazum commented 4 years ago

Maybe it is clang related.

It tells compiler that you really mean assignment inside if conditional clause, not accidentally mistook it with comparison (==).

So writing

if ((a = foo)) {
   /* Some code */
}

is a way to tell the compiler you really mean

if ((a = foo) != 0) {
   /* Some code */
}
shamazmazum commented 4 years ago

I've completed the game with these patches, everything is OK, nothing is broken, if you are concerned.

I would like to send a port to FreeBSD's ports collection for System Shock and want to include my patches. I have two options: either you will accept this request or I will supply it as patches in the port. Please let me know your decision.

speachy commented 4 years ago

Clang is probably defaulting to reporting more warnings than GCC. Enabling -Wall under gcc10 spews warnings left and right, and a great many of them are shut up by the patches in this PR.

Oh, and that 'if (foo = bar)' construct triggers warnings because it's highly suspicious and probably incorrect. However, in the (relatively) rare situations when it is correct, using double-parenthesis 'if ((foo = bar))' makes it explicit.

Interrupt commented 4 years ago

Looks like there's a new conflict, can merge once that is resolved.

shamazmazum commented 4 years ago

Fixed that