Fix some warnings and general cleanup #248

voidanix · 2022-06-17T21:31:04Z

Initialize x, y, z inside vec
Remove volatile keyword from errors counter
Clear objects of now trivial type
Fix possible uninitialization of height
Convert weapon selection system to std::vector
Fix -Wnon-pod-varargs error on clang 14
Fix -Wint-in-bool-context
Document what the sound enums actually are
Fix unneeded parenthesis
Fix uninitialized values in md3 structs
Fix comparison between different signedness
Fix address of array always returning true
Remove set but unused variables
Fix implicit conversion between float and int
Fix shift with negative value
Attempt to fix misleading indentation warning

TheAssassin

Had a first look. Mostly great improvements of the code. I have a few remarks I'd like to discuss, though.

src/engine/irc.cpp

src/engine/renderva.cpp

src/game/ai.cpp

src/game/client.cpp

src/shared/ents.h

src/game/server.cpp

TheAssassin · 2022-06-18T00:27:30Z

src/game/physics.cpp

            {
                const bvec &watercol = getwatercol((curmat == MAT_WATER ? matid : pl->inmaterial) & MATF_INDEX);
-                mattrig(bottom, watercol, 0.5f, int(radius), PHYSMILLIS, 0.25f, PART_SPARK, curmat != MAT_WATER ? S_SPLASH2 : S_SPLASH1);
+                mattrig(bottom, watercol, 0.5f, int(radius), PHYSMILLIS, 0.25f, PART_SPARK, (curmat != MAT_WATER ? S_SPLASH2 : S_SPLASH1));


Just a readability change, right?

Kind of: the compiler is probably interpreting this as MAT_WATER ? S_SPLASH2 : S_SPLASH1 and then comparing against curmat

!= is computed before ?:

@robalni is right here. If you want to make this more explicit (and improve readability), you have to put brackets around the != comparison.

That does not work I'm afraid:

bn/src/game/physics.cpp: In function ‘void physics::updatematerial(physent*, const vec&, const vec&, bool)’: bn/src/game/physics.cpp:1058:115: warning: ‘?:’ using integer constants in boolean context, the expression will always evaluate to ‘true’ [-Wint-in-bool-context] 1058 | mattrig(bottom, watercol, 0.5f, int(radius), PHYSMILLIS, 0.25f, PART_SPARK, (curmat != MAT_WATER) ? S_SPLASH2 : S_SPLASH1); | ^

I think the problem here is that in the definition of the macro mattrig, the last argument mw is used like:

if(mw >= 0)

and this expands to:

if(curmat != MAT_WATER ? S_SPLASH2 : S_SPLASH1 >= 0)

which is the same as:

if( (curmat != MAT_WATER) ? S_SPLASH2 : (S_SPLASH1 >= 0) )

so I think this macro needs to be fixed by adding parentheses around the arguments:

if((mw) >= 0)

and fun fact, when curmat is MAT_WATER then (S_SPLASH1 >= 0) is used and this equals 1 and is the same as S_GUIBACK which means that S_GUIBACK has been played where S_SPLASH1 should have been played.

Awesome catch! Looks a bit ugly now though 👀

Not even worth converting into a function because this macro is used twice in the codebase...

In that case it might just be removed entirely. That tiny bit of code duplication is acceptable.

src/game/game.h

src/engine/main.cpp

src/game/aiman.h

voidanix · 2022-06-18T20:18:27Z

Thanks for the reviews @TheAssassin and @robalni ! Updated the branch

voidanix · 2022-07-09T13:47:15Z

FYI the inrange function now uses constexpr (it will be reused in a later refactor) and the custom NULL macro has been removed

TheAssassin

I found a few more things.

Please also clean up the branch history when you're done.

src/game/client.cpp

TheAssassin · 2022-07-11T00:39:31Z

src/game/client.cpp

+                game::player1.loadweap.end(), i) == game::player1.loadweap.end()) // NOT found
+            {
+                game::player1.loadweap.emplace_back(i);
+                if (game::player1.loadweap.size() >= W_LOADOUT)


Please wrap all conditional code (e.g., branches of an if) in braces to create a proper scope. It is a common issue in refactorings to add lines to existing code without realizing that the scope had ended at the semicolon. This can be avoided by always using a scope.

Would this work?:

if (std::none_of(game::player1.loadweap.begin(), game::player1.loadweap.end(), [=](auto d) { return d == i; }))

I hope there isn't too much overhead if done like this.

This can be avoided by always using a scope.

That is silly and adds unnecessary LoC; even the Kernel's coding style agrees:

Do not unnecessarily use braces where a single statement will do.

Just because the Linux kernel uses it doesn't mean it's beginner friendly. Quite the opposite, really. I've seen this kind of problem too often, so I always use a scope now. It's a pre-emptive measure used to prevent branching problems.

"Saving lines of code" is an argument which I often heard before, but it is not a good one. It might have been relevant, back in the days when there were just 12 lines on one screen. Compacting the code too much like this does not aid readability, quite the opposite, and is often used to compact and therefore kind of "hide" complexity. Look at this line, it's probably too long already.

Python for instance, while not everyone likes the lack of braces, does this very well in general. Whenever the code branches, moving the conditional code on a new line (and therefore adding some indentation as well) is very strongly recommended. You could use the same line, but this is ugly and cumbersome. See https://docs.python-guide.org/writing/style/#one-statement-per-line.

Regarding braces, many popular code styles demand them, there are just few exceptions, mostly for old projects who had established such a style long ago. Not using braces "saves" 1-2 lines depending on the code style. I personally prefer to move the opening one on the same line as the conditional/loop statement. As stated in #248 (comment), this ensures that on future refactorings, new code won't accidentally be added in the hope that it will be conditional as well. Furthermore, this also aids the readability of the code. You can quickly see where branching happens. New variables have a clear scope. You can't be surprised when commenting out lines. You do not have to be careful about the semicolon being the end of the conditional code block.

This is not an obfuscated code competition. The code should be easy to read. Branches should be easily visible. Code should be written with the need for future changes in mind.

Edit: kernel code is among the worst code I've ever seen. This is only partly due to code style problems. It's largely C89-style, even though it's been over 30 years since that standard has been released, and there have been multiple new standards since (even C90 introduced so many positive changes, yet kernel code does not use any, e.g., comments). It uses tabs, which the majority of coders rejects for spaces. It's largely undocumented (i.e., the "what the heck were they thinking when writing that" bit is missing). And these are just a few examples...

src/game/game.h

src/game/physics.cpp

TheAssassin · 2022-07-11T00:42:59Z

src/game/server.cpp

                prev.clear();
            }
-            if(!buf.empty()) setmods(sv_previousmaps, buf.c_str());
+            if(!buf.empty())


While you're at it, please add spaces in between statements such as if and for and the corresponding conditions.

We really need clang-format to auto check changes.

The current style is to not have a space there. I'm not a big fan of mixing styles because the point of a style is the consistency that it gives.

The current style is to not have a space there. I'm not a big fan of mixing styles because the point of a style is the consistency that it gives.

That is the codebase's old style: we aren't fans of mixing stuff either but every new piece of code should be adopting the new style.

src/shared/tools.h

voidanix · 2023-05-04T18:29:32Z

Ping, any updates on this?

Also introduces a new "inrange" function, useful for future refactorings to std::vector

The mattrig macro expands the mw argument at one point into: if (curmat != MAT_WATER ? S_SPLASH2 : S_SPLASH1 >= 0) Co-authored-by: Robert Alm Nilsson <rorialni@gmail.com>

There should be no reason to overwrite this as the C++ spec already specifies it. Compilers nowadays also gift us a dedicated warning for this kind of situation (-Wzero-as-null-pointer-constant). Remove it and specify <cstddef> to still make NULL accessible.

This usage of volatile has been deprecated since C++20: use std::atomic in its place.

Deprecated in C++20

Resolves the ambiguity of implementation defined NULLs Fixes: 8844f42 ("tools: Remove custom NULL")

TheAssassin requested changes Jun 18, 2022

View reviewed changes

TheAssassin mentioned this pull request Jun 18, 2022

Introduce clang-format #249

Open

robalni reviewed Jun 18, 2022

View reviewed changes

src/engine/main.cpp Show resolved Hide resolved

robalni reviewed Jun 18, 2022

View reviewed changes

src/game/aiman.h Outdated Show resolved Hide resolved

voidanix force-pushed the warnings branch from 9e28cce to 569c089 Compare June 18, 2022 20:15

voidanix force-pushed the warnings branch 2 times, most recently from b2151fe to 10f8631 Compare June 24, 2022 08:46

voidanix force-pushed the warnings branch from 10f8631 to 84cd3e4 Compare July 9, 2022 13:45

voidanix requested a review from TheAssassin July 10, 2022 22:59

TheAssassin requested changes Jul 11, 2022

View reviewed changes

voidanix force-pushed the warnings branch from 84cd3e4 to 65d32c5 Compare August 4, 2022 03:20

robalni reviewed Aug 4, 2022

View reviewed changes

src/shared/tools.h Show resolved Hide resolved

voidanix force-pushed the warnings branch from 65d32c5 to 5b28d72 Compare May 4, 2023 17:31

voidanix mentioned this pull request May 11, 2023

C++20-ify #267

Draft

voidanix and others added 13 commits December 8, 2025 02:34

Clear objects of non-trivial type

abfafb6

Fix possible uninitializations

7b25ba9

Convert weapon selection system to std::vector

6e88c24

Also introduces a new "inrange" function, useful for future refactorings to std::vector

Document what the sound enums actually are

6a37a94

Fix unneeded parenthesis

cd3e4bf

Fix comparison between different signedness

9cda6ff

Fix address of array always returning true

ec84636

Remove set but unused variables

082a29c

Fix implicit conversion between float and int

717dc4f

Fix shift with negative value

7a974c6

Attempt to fix misleading indentation warning

fae1ca9

Fix -Wint-in-bool-context

2dc6b60

The mattrig macro expands the mw argument at one point into: if (curmat != MAT_WATER ? S_SPLASH2 : S_SPLASH1 >= 0) Co-authored-by: Robert Alm Nilsson <rorialni@gmail.com>

tools: Remove custom NULL

8844f42

There should be no reason to overwrite this as the C++ spec already specifies it. Compilers nowadays also gift us a dedicated warning for this kind of situation (-Wzero-as-null-pointer-constant). Remove it and specify <cstddef> to still make NULL accessible.

voidanix added 2 commits December 8, 2025 02:34

engine: Use std::atomic, not volatile for errors

3910fab

This usage of volatile has been deprecated since C++20: use std::atomic in its place.

weapons: Prevent arithmetic between different enum types

a39bc24

Deprecated in C++20

voidanix force-pushed the warnings branch from 5b28d72 to 0ddb2e1 Compare December 9, 2025 00:36

Convert all NULLs to nullptr

88793e4

Resolves the ambiguity of implementation defined NULLs Fixes: 8844f42 ("tools: Remove custom NULL")

voidanix force-pushed the warnings branch from 0ddb2e1 to 88793e4 Compare December 9, 2025 00:53

voidanix requested a review from TheAssassin December 9, 2025 00:56

voidanix added this to the 1.7.0 milestone Dec 31, 2025

Fix some warnings and general cleanup #248

Are you sure you want to change the base?

Fix some warnings and general cleanup #248

Uh oh!

Conversation

voidanix commented Jun 17, 2022

Uh oh!

TheAssassin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robalni Jun 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

voidanix commented Jun 18, 2022

Uh oh!

voidanix commented Jul 9, 2022

Uh oh!

TheAssassin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheAssassin Aug 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

voidanix commented May 4, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

robalni Jun 24, 2022 •

edited

Loading

TheAssassin Aug 3, 2022 •

edited

Loading