static analysis of SFML

[minirop]

Hello,
as a test, I've tried PVS-Studio on SFML (master) and, considering I did it correctly, there is not many things to say (which is a good thing).

• Most of the output was about c-style casts (mainly in glloader.cpp).
• Several "variable assigned twice", like:

sfogl_StrToExtMap *entry = NULL;
entry = FindExtEntry(extensionName);

• "The '(FT_RENDER_MODE_NORMAL)' named constant with the value of 0 is used in the bitwise operation. font.cpp:485"
• SoundSource has a copy constructor but no assignment operator.
• And a few about optimization, like : use !string.empty() instead of string != "".
• Last, an UB in stb_image.h. There are bitwise shifts on negative numbers.

That's all folks!

Except the missing operator=(), the rest can be "forgotten" since it's mainly syntax choices¹ (but why the double assignment?)

++

¹: I know C-casts are discouraged, but with C structs and function, the static_cast should "win".

[SuperV1234]

Could you post the complete log?

[minirop]

here it is. it's only an XML file (1.11MB).

[Jabberwocky]

As you noted, it looks to me like SFML passed this test remarkably well!  Thanks for the post, minirop.

[binary1248]

The reason why there are so many warnings in GLLoader.cpp is because, as some might already know, it is an automatically generated loader... in C. I already went ahead and modified it a lot mostly getting rid of things we don't use anyway, but didn't really dig into the finer details of the code that remained. I guess now that I realize that there might be reason to look into it more, I might go ahead and fix up most of those warnings.

[Hiura]

Nice report. 

I'll create a PR for the SoundSource bit.

Regarding FT_RENDER_MODE_NORMAL in Font.cpp, it's better to keep it that way.

As for null string testing, I'd rather keep it that way since it's more idiomatic. But does someone disagree?

[Nexus]

• "The '(FT_RENDER_MODE_NORMAL)' named constant with the value of 0 is used in the bitwise operation. font.cpp:485"

How does that line look? Because in line 485 (master), there are different constants:

if (FT_Load_Char(face, codePoint, FT_LOAD_TARGET_NORMAL | FT_LOAD_FORCE_AUTOHINT) != 0)

(but why the double assignment?)

The separate declaration and initialization is of course pointless, it can be fixed too.
Do you have all the occurrences in text format?

I guess now that I realize that there might be reason to look into it more, I might go ahead and fix up most of those warnings.

Don't we have to do everything again if we change something and regenerate GLLoader.cpp? If it doesn't take long, fine, but let's not waste our time on fixing generated files just for cosmetic reasons

As for null string testing, I'd rather keep it that way since it's more idiomatic. But does someone disagree?

Is it really more idiomatic? After all, the empty() method exists for only this purpose

[binary1248]

Don't we have to do everything again if we change something and regenerate GLLoader.cpp? If it doesn't take long, fine, but let's not waste our time on fixing generated files just for cosmetic reasons

Well, basically any time anybody adds (or maybe even removes?) extensions, what they would have to do is generate the verbatim GLLoader files and extract the bits and pieces that are to be added to SFML's modified version.

This might seem like a lot of work at first glance, but trust me, it really isn't. If more than 100 lines get changed in those files for what I consider a "small extension", then something probably went wrong. Typically the number of lines that get changed is anywhere from 10 to 40 for "average sized extensions".

You can take a look at the GL 1.1 diff. When adding an extension, changes only have to be made in 7 different places within the files. I can do this in a matter of minutes, no idea how long someone else would take.

[minirop]

(but why the double assignment?)

The separate declaration and initialization is of course pointless, it can be fixed too.
Do you have all the occurrences in text format?

all in glloader.cpp, L385/386, L463/464 and L496/502.
looking at the last one make me think it's probably for pre-C99 compatibility (declare all variable then assign them)

[Hiura]

As for null string testing, I'd rather keep it that way since it's more idiomatic. But does someone disagree?

Is it really more idiomatic? After all, the empty() method exists for only this purpose

Well, it's a question of taste, or how you see `std::string`, in the end, true. But I think the `empty` function is here because you can see it as a container. Personally, I see it as a string so checking for emptiness is not that conventional to me.

[minirop]

I tried cppcheck to compare. There are a few "unused variable" like MainAndroid.cpp:L125, lJavaVM is never used, and a few "the score of the variable 'XXX' can be reduced".
Only one warning stands out from the rest, Window\Unix\GlxContext.cpp:L56,57, the initializer list is in the wrong order.