GNOME Bugzilla – Bug 783214
Add a static analysis tool as a make target
Last modified: 2017-07-06 02:38:35 UTC
It is harmless, it changes nothing in a regular development workflow, and can be useful for some people, I'm proposing the patch. * It does static code analysis and output warnings like these: ------- [gi/boxed.cpp:1072]: (style) The scope of the variable 'first_constructor' can be reduced. [gi/closure.cpp:105]: (style) C-style pointer casting ------- * The patch itself is not really useful. Anyway, it documents how to run cppcheck in a sane way. * My goal is QA; I mean, it is better to discuss the warnings than to merge the patch.
Created attachment 352824 [details] [review] Static analysis using cppcheck
Claudio, I'm sorry it's taking me so long to review this patch and the sanitizer patch. I haven't forgotten about it, just busy!
Review of attachment 352824 [details] [review]: Thanks for the patch! It looks like most of the style warnings are harmless, a few are false positives, but cppcheck does find one real bug! "[test/gjs-test-coverage.cpp:905]: (warning) %i in format string (no. 1) requires 'int *' but the argument type is 'unsigned int *'." Would you like to do the honors? ::: Makefile.am @@ +212,3 @@ +# +cppcheck: + $(CPPCHECK) --enable=all --force -q . `.` should instead be `$(top_srcdir)` so that it will work with srcdir != builddir. Also looks like it should have `-I $(top_srcdir) -I $(top_builddir)` to find all the includes.
Related story, at one time I intended to get GJS working with Coverity Scan, but after I created a project on their website it never did the scan, and my questions to their support went unanswered...
One more comment; consider adding a paragraph in doc/Hacking.md telling about this new developer feature!
Created attachment 354059 [details] [review] Fix for a wrong argument
Created attachment 354060 [details] [review] Static analysis using cppcheck
(In reply to Philip Chimento from comment #4) > Related story, at one time I intended to get GJS working with Coverity Scan, > but after I created a project on their website it never did the scan, and my > questions to their support went unanswered... I am a fan of these kind of tools. They do help.
Review of attachment 354059 [details] [review]: +1, thanks!
Review of attachment 354060 [details] [review]: Just a few comments, if you agree with them then I can simply make the change and commit it without having to upload a new patch. ::: doc/Hacking.md @@ +95,3 @@ +To execute cppcheck, a static code analysis tool for the C and C++, run: +```sh +jhbuild run --in-builddir=gjs make cppcheck "jhbuild make cppcheck" works for me. If you agree, then let's change it to that, it corresponds with how other commands in this guide are run. @@ +119,3 @@ (replace `X.Y.Z` with the version number, e.g. `1.48.0`) +[jhbuild] https://wiki.gnome.org/HowDoI/Jhbuild Unrelated change?
Review of attachment 354060 [details] [review]: ::: Makefile.am @@ +212,3 @@ +# +cppcheck: + $(CPPCHECK) --enable=warning,performance,portability,information,missingInclude --force -q $(top_srcdir) -I $(top_srcdir) -I $(top_builddir) -I $(includedir) One more thing, you have -I $(top_srcdir) twice, and should probably omit -I $(includedir) since cppcheck says it "does not need system headers to work properly"
(In reply to Philip Chimento from comment #10) > Review of attachment 354060 [details] [review] [review]: > > Just a few comments, if you agree with them then I can simply make the > change and commit it without having to upload a new patch. > > ::: doc/Hacking.md > @@ +95,3 @@ > +To execute cppcheck, a static code analysis tool for the C and C++, run: > +```sh > +jhbuild run --in-builddir=gjs make cppcheck > > "jhbuild make cppcheck" works for me. If you agree, then let's change it to > that, it corresponds with how other commands in this guide are run. It doesn't work for me. - How is your jhbuild configuration file? Think about: jhbuild can handle a lot of modules (and Makefiles). Which one it should use if you do not say 'gjs' somehow? > > @@ +119,3 @@ > (replace `X.Y.Z` with the version number, e.g. `1.48.0`) > > +[jhbuild] https://wiki.gnome.org/HowDoI/Jhbuild > > Unrelated change? Yes, Git insisted on this. We can remove it when I get a real computer.
(In reply to Philip Chimento from comment #11) > Review of attachment 354060 [details] [review] [review]: > > ::: Makefile.am > @@ +212,3 @@ > +# > +cppcheck: > + $(CPPCHECK) > --enable=warning,performance,portability,information,missingInclude --force > -q $(top_srcdir) -I $(top_srcdir) -I $(top_builddir) -I $(includedir) > > One more thing, you have -I $(top_srcdir) twice They are different stuff (w/ and w/o -I), but I agree that the '-I' relative to source files is not mandatory. > and should probably omit -I $(includedir) since cppcheck says it "does > not need system headers to work properly" Here $(includedir) is .../jhbuild/install/include. Is it only for system header files?
(In reply to Philip Chimento from comment #10) > > +[jhbuild] https://wiki.gnome.org/HowDoI/Jhbuild > > Unrelated change? Blame gedit. But, the committed file is not POSIX compliant (and gedit just fixed this fact). Maybe break this into two different commits?
(In reply to Claudio André from comment #12) > (In reply to Philip Chimento from comment #10) > > Review of attachment 354060 [details] [review] [review] [review]: > > > > Just a few comments, if you agree with them then I can simply make the > > change and commit it without having to upload a new patch. > > > > ::: doc/Hacking.md > > @@ +95,3 @@ > > +To execute cppcheck, a static code analysis tool for the C and C++, run: > > +```sh > > +jhbuild run --in-builddir=gjs make cppcheck > > > > "jhbuild make cppcheck" works for me. If you agree, then let's change it to > > that, it corresponds with how other commands in this guide are run. > > > It doesn't work for me. > - How is your jhbuild configuration file? Think about: jhbuild can handle a > lot of modules (and Makefiles). Which one it should use if you do not say > 'gjs' somehow? I see where we are disconnected - all the instructions in that document assume you are in the gjs checkout dir already. (In reply to Claudio André from comment #13) > (In reply to Philip Chimento from comment #11) > > and should probably omit -I $(includedir) since cppcheck says it "does > > not need system headers to work properly" > > Here $(includedir) is .../jhbuild/install/include. Is it only for system > header files? Hmm, I considered all GLib, mozjs, etc. headers "system headers", but maybe that is not correct. I don't remember getting any extra diagnostic messages with the includedir in place. Did you? (In reply to Claudio André from comment #14) > (In reply to Philip Chimento from comment #10) > > > > +[jhbuild] https://wiki.gnome.org/HowDoI/Jhbuild > > > > Unrelated change? > > Blame gedit. But, the committed file is not POSIX compliant (and gedit just > fixed this fact). > > Maybe break this into two different commits? No, not necessary :-) Go ahead with the extra blank line change.
Created attachment 354797 [details] [review] Static analysis using cppcheck
Review of attachment 354797 [details] [review]: Great, thanks!