Reconstructed branch from zipped set of patches dumped in #928

[illwieckz]

Reconstructed branch from zipped set of patches dumped in #928:

• https://github.com/DaemonEngine/Daemon/issues/928#issue-1892984160

The code was not read.

This is not reviewed, at all, so it is not approved.

All I did was to apply patches one by one by hand and rewrite the commits by hand when there have been patch application rejects, which happened more than once since the target reference for those patches is unknown.

It is said that “it seems to fix“ some “memory leak in both cvar systems (Cvar and cvar_t)”.

[slipher]

This is too big and needs to be split up or have some commits removed. Most commits obviously don't have anything to do with memory leaks.

BTW I doubt there is any real leak fixed here at all. It's just that when exiting the program we don't free all the pointers (which is pointless because the program is exiting). The only purpose in freeing them is to appease sanitizer programs.

[DolceTriade]

For the code that's linked into the gamelogic, it's important to free memory for the shared lib use case

[slipher]

For the code that's linked into the gamelogic, it's important to free memory for the shared lib use case

The code in question is in src/engine/

[DolceTriade]

I thought the changes to cm/ would be linked into the game logic?

[slipher]

I thought the changes to cm/ would be linked into the game logic?

There are changes to cm/ but they are very trivial and unrelated to memory management. There is just one commit that purports to fix a leak - I'm talking about that one.

[illwieckz]

BTW I doubt there is any real leak fixed here at all. It's just that when exiting the program we don't free all the pointers (which is pointless because the program is exiting). The only purpose in freeing them is to appease sanitizer programs.

Yeah we had that talk when doing https://github.com/DaemonEngine/Daemon/pull/897 and other things.

Anyway, if we have commits that removes false positive from ASAN that may even help us to better spot true positive, so I won't mind merging them.

This is too big and needs to be split up or have some commits removed.

Let's say each commit in this branch is a PR by itself but I will not open a PR per commit. If someones says something like LGTM for “fix -Wunused-macro”, I will merge the said commit, moving steps by steps.

[slipher]

Good to merge:

• R_LoadDDSImageData is static LGTM
• static things LGTM

Wouldn't merge:

• fix -Wunused-macro looks pretty ugly (and actively wrong with the libpng one)
• fix missing-variable-declaration looks bad
• more static stuff looks bad because writing the two lines foo bar; extern foo bar; one right after the other is nonsensical
• I dislike fix uninitialised-condition. This is more a matter of taste

There are good reasons we don't have certain compiler warnings enabled. Some of them just encourage making the code worse.

[illwieckz]

Thanks. About fix uninitialised-condition, it looks like changes in src/engine/framework/BaseCommands.cpp and src/engine/server/sv_client.cpp are just working around the fact the compiler doesn't know that Str::ParseInt modifies the argument. I don't know if there is a c++ way of telling the compiler the function does that. But what is your opinion about the change in src/engine/renderer/tr_scene.cpp ?

[illwieckz]

• more static stuff looks bad because writing the two lines foo bar; extern foo bar; one right after the other is nonsensical

I guess you talk about:

-cvar_t                     *r_allowResize; // make window resizable
+extern cvar_t *r_allowResize; // make window resizable
+cvar_t *r_allowResize;

I guess the second line is an elaborated kind of typo. I can fix that. Have you seen something else? Can I merge this commit if I delete the second line?

[illwieckz]

fix -Wunused-macro looks pretty ugly (and actively wrong with the libpng one)

Right, it looks like the change in libpng will just make the game to not build on some distros because with this change “it (still) works for me” on another distro.

But what do you think about the #ifndef BUILD_VM in src/common/FileSystem.cpp and #ifdef _WIN32 in src/engine/framework/VirtualMachine.cpp?

[slipher]

Thanks. About fix uninitialised-condition, it looks like changes in src/engine/framework/BaseCommands.cpp and src/engine/server/sv_client.cpp are just working around the fact the compiler doesn't know that Str::ParseInt modifies the argument. I don't know if there is a c++ way of telling the compiler the function does that. But what is your opinion about the change in src/engine/renderer/tr_scene.cpp ?

It appears that the existing code has an off-by-one bug, where if the number of tests is MAX_VISTESTS - 1 and you attempt to register another one, the last slot will be recycled by mistake. The change makes the consequences of this bug worse by falling off the end of the function instead of reusing the last slot.

I guess you talk about:

-cvar_t                     *r_allowResize; // make window resizable
+extern cvar_t *r_allowResize; // make window resizable
+cvar_t *r_allowResize;

I guess the second line is an elaborated kind of typo. I can fix that. Have you seen something else? Can I merge this commit if I delete the second line?

It is clearly intentional as the same pattern is repeated several times in the commit. Apparently to appease some obscure warning. No, the commit is still no good if you delete the second line.

But what do you think about the #ifndef BUILD_VM in src/common/FileSystem.cpp and #ifdef _WIN32 in src/engine/framework/VirtualMachine.cpp?

I think the BUILD_VM one is silly and this commit is a waste of time.

[ghost]

Don't worry slipher, next time I have memleaks or other fixes like that, I'll just keep them for myself, and to make your stats look better, I might also avoid reporting a bug. I won't put code that is "nonsensical", "looks bad", etc under your eyes even if it contains a fix for a memory leak that is so trivial to prove (run with asan, nothing complicated) that it simply shows how much tools are used to sanitize the codebase: not at all, as you even admit by "not enabling some warnings". That's not the way to do things. The way to do things is to enable all warnings, and only disable those which do not make sense. But this would end up with thousands of warnings in this codebase, and daemon's warnings even propagate to the gamelogic because there's that much (warning) code in headers.

The more I know Unvanquished, the more I think the only stuff that have value for contributing back are changes in what mappers can do. Any other stuff just is a waste of time, and I'm sure you know why.

[slipher]

OK to merge:

• remove unused global vars LGTM
• move some cvars to static LGTM

Needs fixes: fix a 200 Byte leak per cvar - using unique_ptr for cvarRecord_t is a good cleanup, but there's a problem with the bit migrating the ccvar strings (latchedString, resetString etc.) to std::string. Sometimes they have optional string semantics, with code like if (cvar.flags & CVAR_LATCH and cvar.ccvar.latchedString). Also looks like this commit won't compile on its own.