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

@@ -1055,7 +1055,7 @@ namespace physics
            if(curmat == MAT_WATER || oldmat == MAT_WATER)
            {
                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.

TheAssassin · 2022-07-11T00:36:48Z

src/game/client.cpp

+        game::player1.loadweap.clear();
+        for (const auto i : items) {
+            if (std::find(game::player1.loadweap.begin(),
+                game::player1.loadweap.end(), i) == game::player1.loadweap.end()) // NOT found


I think the comment is not needed, even less experienced C++ programmers should know this idiom.

Also I'm pretty much the opposite of a fan of inline comments. They just increase the line length for no apparent reason and are hard to spot/find/read as well.

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

@@ -3395,7 +3395,8 @@ namespace server
                }
                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.

robalni · 2022-08-04T08:21:54Z

src/shared/tools.h

-#ifdef NULL
-#undef NULL
-#endif
-#define NULL 0


What is the reason to replace a well defined NULL value with an implementation defined value? As we can read on https://en.cppreference.com/w/cpp/types/NULL the value of NULL can be 0 or nullptr. These two values have different sizes on 64 bit computers and not knowing which one it is can cause problems in some situations, like when using varargs where you specify which type you want to read but NULL makes sure that you don't know which type you give to the function.

I think a better change (if we want to change it) might be to define it as nullptr or (void *)0 or (size_t)0 to make sure that it has the same size as a pointer because NULL is often used where you would use a pointer. Another solution could be to ban NULL (but then we would have to change in a lot of places) by defining it to a value that will generate a compilation error when used. That could be done if we want to avoid confusion that people think they are using the standard library NULL when they are really using our NULL.

First of all, this partly solves the -Wzero-as-null-pointer-constant warning which is (yet?) not enabled in BN.

NULL is often used there you would use a pointer.

Almost always, actually: indeed using nullptr would totally eliminate the issue of weird conversions.

like when using varargs where you specify which type you want to read but NULL makes sure that you don't know which type you give to the function.

I fail to see how different sizes actually makes things act weirdly, but using nullptr (which must be passable through varargs) would be nicer too.

Another solution could be to ban NULL

I encourage everybody to use nullptr, but some files are still using pure C here: once a conversion of sorts is completed, then sure.

FWIW, while my NULL expands to __null, I know that MS's (and MINGW's, according to libcxx) expands to 0: see this commit as a stop gap, because I would much rather see some platforms spit out warnings about NULL than giving an error.

I fail to see how different sizes actually makes things act weirdly

It's something I read somewhere but I'm not able to reproduce it so I don't know if it's actually a problem.

but some files are still using pure C

If we make a new constant like #define null ((void *)0) that can be used in C too.

my NULL expands to __null

And my NULL expands to ((void *)0) on Debian/amd64/gcc :)

I would much rather see some platforms spit out warnings about NULL than giving an error

Yes, it's probably a bad idea to hard ban NULL because there can be code (like in the standard library itself) that uses NULL that we don't easily control and then that code would break. For the same reason it might actually be a good idea to leave NULL defined as in the standard library because maybe there is some code in there that has expectations on the definition of NULL. I don't know.

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 <[email protected]>

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

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

Ping, any updates on this?

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

voidanix and others added 15 commits May 4, 2023 19:21

Clear objects of non-trivial type

4040038

Fix possible uninitializations

7272660

Convert weapon selection system to std::vector

2ba65bb

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

Document what the sound enums actually are

f07eb85

Fix unneeded parenthesis

3b800a4

Fix comparison between different signedness

c8143e6

Fix address of array always returning true

bb50c90

Remove set but unused variables

af57554

Fix implicit conversion between float and int

dd33dfb

Fix shift with negative value

d7b723e

Attempt to fix misleading indentation warning

3eb9dac

Fix -Wint-in-bool-context

64ae458

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 <[email protected]>

tools: Remove custom NULL

d938486

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.

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

7cdfbc2

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

weapons: Prevent arithmetic between different enum types

5b28d72

Deprecated in C++20

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

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix some warnings and general cleanup #248

Fix some warnings and general cleanup #248

voidanix commented Jun 17, 2022

TheAssassin left a comment

TheAssassin Jun 18, 2022

voidanix Jun 18, 2022

robalni Jun 18, 2022

TheAssassin Jun 18, 2022

voidanix Jun 24, 2022

robalni Jun 24, 2022 •

edited

Loading

voidanix Jun 24, 2022

TheAssassin Aug 3, 2022

voidanix commented Jun 18, 2022

voidanix commented Jul 9, 2022

TheAssassin left a comment

TheAssassin Jul 11, 2022

TheAssassin Jul 11, 2022

voidanix Aug 1, 2022

voidanix Aug 1, 2022

TheAssassin Aug 3, 2022 •

edited

Loading

TheAssassin Jul 11, 2022

robalni Jul 11, 2022

voidanix Aug 1, 2022

robalni Aug 4, 2022 •

edited

Loading

voidanix Aug 5, 2022

robalni Aug 5, 2022

voidanix commented May 4, 2023

Fix some warnings and general cleanup #248

Are you sure you want to change the base?

Fix some warnings and general cleanup #248

Conversation

voidanix commented Jun 17, 2022

TheAssassin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robalni Jun 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

voidanix commented Jun 18, 2022

voidanix commented Jul 9, 2022

TheAssassin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheAssassin Aug 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robalni Aug 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

voidanix commented May 4, 2023

robalni Jun 24, 2022 •

edited

Loading

TheAssassin Aug 3, 2022 •

edited

Loading

robalni Aug 4, 2022 •

edited

Loading