By Anonymous
Date 2017-04-22 18:05
Disclaimer: I'm new to openclonk. Please inform me if this needs to be submitted somewhere else.
I tried going through the history in the forum and found this thread from 2011. They've used cppcheck to analyze the code.
If this has been attempted afterwards and was discarded for some reason (it would be nice if someone tells me why), I apologize for bringing it up again.
Since then, there have been some advancements and now clang provides an analyzer along with the compiler distribution (MSVC - cl.exe does as well, but I still suggest using clang). We can use the command-line option
A sample report from clang's analysis on src/graphics/C4Draw.cpp:
I liked
A handpicked sample from its report:
Its "fix" option further simplifies the process. It can automatically apply trivial fixes which are only mechanical changes.
If you like the idea and want to pursue this, please let me know. I can share the commands I ran. I used Windows; I'm sure they'll be similar for linux or macOS if not much cleaner.
Also, if approved, this could potentially be added to the Travis-CI config to run automatically.
I tried going through the history in the forum and found this thread from 2011. They've used cppcheck to analyze the code.
If this has been attempted afterwards and was discarded for some reason (it would be nice if someone tells me why), I apologize for bringing it up again.
Since then, there have been some advancements and now clang provides an analyzer along with the compiler distribution (MSVC - cl.exe does as well, but I still suggest using clang). We can use the command-line option
--analyze to "Run the static analyzer" (taken as-is from clang -help). It can list the instances of potential null-deference, unused variable initialization, etc.A sample report from clang's analysis on src/graphics/C4Draw.cpp:
In file included from T:\\GitRepo\\openclonk\\src\\graphics\\C4Draw.cpp:33:
..\\..\\src\lib/StdColors.h(121,26): warning: Value stored to 'dwSrcClr' is never read
BYTE sR=BYTE(dwSrcClr); dwSrcClr=dwSrcClr>>8;
^ ~~~~~~~~~~~
..\\..\\src\lib/StdColors.h(125,26): warning: Value stored to 'dwDstClr' is never read
BYTE dR=BYTE(dwDstClr); dwDstClr=dwDstClr>>8;
^ ~~~~~~~~~~~
T:\\GitRepo\\openclonk\\src\\graphics\\C4Draw.cpp(104,32): warning: Access to field 'Wdt' results in a dereference of a null pointer (loaded from field 'sfcPattern32')
CachedPattern = new uint32_t[sfcPattern32->Wdt * sfcPattern32->Hgt];
^~~~~~~~~~~~~~~~~
T:\\GitRepo\\openclonk\\src\\graphics\\C4Draw.cpp(340,8): warning: Value stored to 'scaleX' during its initialization is never read
float scaleX = twdt/fwdt;
^~~~~~ ~~~~~~~~~
T:\\GitRepo\\openclonk\\src\\graphics\\C4Draw.cpp(341,8): warning: Value stored to 'scaleY' during its initialization is never read
float scaleY = thgt/fhgt;
^~~~~~ ~~~~~~~~~
T:\\GitRepo\\openclonk\\src\\graphics\\C4Draw.cpp(363,6): warning: Value stored to 'iTexSizeX' during its initialization is never read
int iTexSizeX=sfcSource->iTexSize;
^~~~~~~~~ ~~~~~~~~~~~~~~~~~~~
T:\\GitRepo\\openclonk\\src\\graphics\\C4Draw.cpp(364,6): warning: Value stored to 'iTexSizeY' during its initialization is never read
int iTexSizeY=sfcSource->iTexSize;
^~~~~~~~~ ~~~~~~~~~~~~~~~~~~~
7 warnings generated.
I liked
clang-tidy's report even more. Including the analyzer's checks, it has more checks to keep the code "tidy". My favorite is the modernize class of checks. It suggests changes to use modern C++ constructs (I read somewhere it suggests only C++11 stuff).A handpicked sample from its report:
T:\GitRepo\openclonk\src\graphics\C4Draw.cpp:109:19: warning: use nullptr [modernize-use-nullptr]
CachedPattern = 0;
^
nullptr
T:\GitRepo\openclonk\src\graphics\C4Draw.cpp:718:4: warning: use auto when initializing with a template cast to avoid duplicating the type name [modernize-use-auto]
int32_t gray = Clamp<int32_t>((r + g + b) / 3 + iOffset, 0, 255);
^
auto
T:\GitRepo\openclonk\src\graphics\C4Draw.cpp:765:2: warning: use range-based for loop instead [modernize-loop-convert]
for (int i = 0; i < C4MaxGammaRamps; i++) {
Its "fix" option further simplifies the process. It can automatically apply trivial fixes which are only mechanical changes.
If you like the idea and want to pursue this, please let me know. I can share the commands I ran. I used Windows; I'm sure they'll be similar for linux or macOS if not much cleaner.
Also, if approved, this could potentially be added to the Travis-CI config to run automatically.
Hi Anonymous,thanks for looking into the OpenClonk project! Static analysis is actually already done by the autobuilds. For example, here is the output of the latest Windows build:
https://autobuild.openclonk.org/builds/3d91671d-44fb-4634-b206-c36a6a876a7c/
The warnings about unused variables are visible in yellow; the places you mentioned are already there. Personally, I would love to be warning-free (and turn off warnings that don't make sense / we don't intend to react to), so each new warning can be checked immediately. Unfortunately, noone works on that for now. If you want to provide a patch, please go ahead! It's usually posted as a pull request on github and then advertised here.
The 0 versus nullptr is probably a good change. I'm not sure about the "auto" versus "int32_t", but don't care too much either way. Using range-based for everywhere also sounds like a pretty tiresome change. Particularly because we use some non-STL container classes in some places that do not support this kind of loop (yet).
> Static analysis is actually already done by the autobuilds.
That only highlights the warnings the compiler emits by default, which is nowhere close to what a real static analyzer does.
> Particularly because we use some non-STL container classes in some places that do not support this kind of loop (yet).
clang-modernize isn't going to complain about those cases.
![[in]](/mwf/flags/in.png "India")
Hi, I'm Tushar. I posted the original message. I don't know if there is a way to edit the sender id for that now.
I saw the build output in the link. I agree with @Isilkor, it looks like the warnings emitted by the compiler are highlighted in yellow. While a static analyzer does catch those, a good one usually has more to offer. I feel I've sold clang short.
I observed that clang-tidy covered everything clang analyzer was showing, and it offered the modernize checks. To make things more interesting, it offers an option to apply automatic fixes.
The hacking:
I patched up a script to run clang-tidy on every cpp file referenced from the CMake files. I didn't allow it to modify the header files - which can be done next - or files in the thirdparty directory.
The script ran for about 2 hours on my laptop (the problems include - no parallel processing, using Windows, you name it). There are more improvements possible to make it faster and portable. Do let me know if you want to see the first version.
The code was mostly in working condition, except for a few things:
- Some macros got messed up. I reverted those by hand.
- Some fixes got applied twice
- add
- call
-
- extra white-space
- I'm fixing these as I find them.
The result:
At completion, the script had modified 187 of 229 cpp files.
The build from Travis succeeded. Sadly, I have no clue if the CI build for Windows succeeded or not. It is building fine on my local machine.
I branched from master, you can see the comparison.
Or, if that becomes outdated, please refer to the commit.
Note that these are only the automatic fixes.
The conundrum:
The change looks big (at least to me). And to top it off, it has some machine errors. I'm not sure if I should even try to create a PR with this commit. My guess: it would be closed by the first moderator seeing it.
Any suggestions for me?
I saw the build output in the link. I agree with @Isilkor, it looks like the warnings emitted by the compiler are highlighted in yellow. While a static analyzer does catch those, a good one usually has more to offer. I feel I've sold clang short.
I observed that clang-tidy covered everything clang analyzer was showing, and it offered the modernize checks. To make things more interesting, it offers an option to apply automatic fixes.
The hacking:
I patched up a script to run clang-tidy on every cpp file referenced from the CMake files. I didn't allow it to modify the header files - which can be done next - or files in the thirdparty directory.
The script ran for about 2 hours on my laptop (the problems include - no parallel processing, using Windows, you name it). There are more improvements possible to make it faster and portable. Do let me know if you want to see the first version.
The code was mostly in working condition, except for a few things:
- Some macros got messed up. I reverted those by hand.
- Some fixes got applied twice
- add
override- call
std::move-
include <utility>- extra white-space
- I'm fixing these as I find them.
The result:
At completion, the script had modified 187 of 229 cpp files.
The build from Travis succeeded. Sadly, I have no clue if the CI build for Windows succeeded or not. It is building fine on my local machine.
I branched from master, you can see the comparison.
Or, if that becomes outdated, please refer to the commit.
Note that these are only the automatic fixes.
clang-tidy still gives warnings which need to be manually addressed.The conundrum:
The change looks big (at least to me). And to top it off, it has some machine errors. I'm not sure if I should even try to create a PR with this commit. My guess: it would be closed by the first moderator seeing it.
Any suggestions for me?
![[se]](/mwf/flags/se.png "Sweden")
I like this in general, but I feel like we shouldn't replace types with auto everywhere. I'm not at all against using auto, but I don't feel like we win anything by converting existing type declarations.Is it possible to disable individual changes like this?
We've had some large "style fix" commits in the past, so another one is probably fine.
Yes, it is possible to disable the checks individually. As a matter of fact, I've already disabled
I'll run a modified script with
Just as an added input, can you tell me which among the following look out of place so I can disable those also.
modernize-use-using in my script, because it fails more often than not. My guess: my version of clang-tidy is missing this patch.I'll run a modified script with
modernize-use-auto disabled as well. Wish me luck. :D. I'll push another commit to a different branch with those changes.Just as an added input, can you tell me which among the following look out of place so I can disable those also.
modernize-avoid-bind
modernize-deprecated-headers
modernize-loop-convert
modernize-make-shared
modernize-make-unique
modernize-pass-by-value
modernize-raw-string-literal
modernize-redundant-void-arg
modernize-replace-auto-ptr
modernize-shrink-to-fit
modernize-use-auto
modernize-use-bool-literals
modernize-use-default-member-init
modernize-use-emplace
modernize-use-equals-default
modernize-use-equals-delete
modernize-use-nullptr
modernize-use-override
modernize-use-transparent-functors
modernize-use-using
Alright, I had a thought better than repeating the whole "running the script and manually fixing things I've fixed already" business.
Just a replacement of a type with
Well, it seems to have worked:
The comparison.
The commit.
The Travis build.
As expected, the diff is smaller (although it still looks big to me). As an added benefit it leaves
Let me know if you want me to run the procedure with
Just a replacement of a type with
auto can be picked up by a regex search in the patch file. The change can be reverted, and the patch then applied on the original files.Well, it seems to have worked:
The comparison.
The commit.
The Travis build.
As expected, the diff is smaller (although it still looks big to me). As an added benefit it leaves
auto in places where it can be called syntactic sugar (some range-for loops for example). I hope that is acceptable.Let me know if you want me to run the procedure with
modernize-use-auto disabled to get rid of all usages of auto.
Cool, I like this better now. Iterators are one of the cases where auto is very useful. I think the patch needs a full manual review before merging which I'm happy to do once I'm back from vacation next week (in case nobody else does it before that).
Feel free to do a pull request once you're happy with the changes.
![[de]](/mwf/flags/de.png "Germany")
Generally, I like static analysis. The disadvantage of automated changes like that is that if there is an error, it will be pretty hard to spot (because you f.e. cannot bisect).Also, what you probably noticed - the automated changes include stuff like
replacing
StdCompilerConfigRead CfgRead(HKEY_CURRENT_USER, "Software\\" C4CFG_Company "\\" C4ENGINENAME);with
StdCompilerConfigRead CfgRead(HKEY_CURRENT_USER, R"(Software\OpenClonk Project\OpenClonk)");
Round 2
More hacking:
I modified the script to run in batches grouped by sub-directories, allowing fixes in the header files in the same sub-directory.
The machine errors were worse headers included in multiple compilation units get patched multiple times.
I've addressed all the errors introduced by the automatic fixes.
The result:
The diff is larger than the one I got before. As before, I started from master, you can see the comparison.
The Travis build job succeeded. I am not aware of the Windows or Darwin CI status. It builds fine on my local Windows machine.
As before, these are only the automatic fixes (with more manual work to fix the errors created by those :D).
The request
Please let me know if there are any suggestions/comments for the same. I can open a PR for reviews if required.
Noone is going to check it all, but changes look OK. What's up with the umlauts (like ΓΌ) in comments being re-encoded? github displays the old umlauts correctly, but after your change it puts a question mark there. Is it changing the encoding to UTF8? Or is it just killing the symbol?
Thanks for pointing it. It changed the encoding to UTF-8, but did not encode the non-ASCII characters properly. I ran a search for all such characters in my diff, the only instances are in
I've opened the PR. We can continue with the review/discussion on the same.
lib/StdCompiler.cpp. I've replaced the character with the UTF-8 equivalent - copied from platform/C4App.h.I've opened the PR. We can continue with the review/discussion on the same.
![[us]](/mwf/flags/us.png "United States")
Powered by mwForum 2.29.7 © 1999-2015 Markus Wichitill
