Merge lp:~widelands-dev/widelands/codecheck into lp:widelands

[SirVer]

Test more on travis

- Enable building documentation with warnings as errors on travis.
- Enable running running the test suite of our lints on travis.
- Remove dynamic_cast<> lint - I no longer am a fan of upcast().
- Fix test suite & docs errors.
- Use mathjax for latex in docs instead of imgmath.

[SirVer (sirver)]

This is not yet for review. I want to see this fail on travis before fixing the errors.

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 2416. State: failed. Details: https://travis-ci.org/widelands/widelands/builds/249429555.
Appveyor build 2243. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_codecheck-2243.

[SirVer (sirver)]

Okay, this is ready for review now.

[Hans Joachim Desserud (hjd)]

Nice intiative :)

Btw, I experimented a bit with cppcheck here the other day. We don't need to decide it in this merge proposal, but that might be something to consider to get more checks run at build time. Now, the full report with all checks enabled takes hours, so that's an obvious no-go. However, the default set (don't know how minimal this is) runs through the source code in 20-25 seconds on my laptop:

$ time cppcheck --verbose --quiet -i src/third_party/ src/
[src/editor/tools/info_tool.cc:179]: (error) Memory leak: multiline_textarea
[src/ui_fsmenu/launch_mpg.cc:71]: (error) Memory leak: btn
[src/wui/watchwindow.cc:125]: (error) Memory leak: closebtn

real 0m23,087s
user 0m22,848s
sys 0m0,052s

Remove --quiet to see more details on what it processes. Also note that I was able to ignore issues in third_party modules, something I also tried (but failed to get working) with the full run in utils/create_cppcheck_report.

As mentioned above I don't know how much this actually checks, though I suppose there should be a list list of which ones are enabled by default. It is also possible to enable additional checks one by one, but the more we add the more the run time is going to increase. Lastly (and a bit related) to the question below is whether this would fail the build or not. If someone needs to read the logs to discover new issues we might just run a check every once in a while instead of each and every build. Thoughts?

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 2424. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/249640208.
Appveyor build 2251. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_codecheck-2251.

[SirVer (sirver)]

> Nice intiative :)

Thanks, but this also scratches two concrete itches I had:
1) I added a new lint in another branch and found out that you cannot reliably run the lint tests anymore because some fail.
2) Errors in the docs trigger an email per hour that ends up in my inbox. There are currently errors in the docs and I'd like the email flood to stop.

cppcheck is orthogonal to these things - but I agree that it would be useful.

And yes, errors in the docs and in the lint testsuite will now break the build.

[kaputtnik (franku)]

> - Use mathjax for latex in docs instead of imgmath.

We may need to adjust the website code to get pretty output for formulas then. See the image with current (above) and after the switch to mathjax (bottom) for a comparison: http://image.ibb.co/dd5cPa/switch_to_mathjax.jpg

[kaputtnik (franku)]

I've made a website-branch to fix displaying of formulas when switching to mathjax:

https://code.launchpad.net/~widelands-dev/widelands-website/mathjax_for_documentation

This works fine except for the coloring of formulas. As far i remember i added the coloring for better contrast, but my test with the above website-branch shows good contrast for the formulas. So i don't know if we want the coloring anymore. If not you could remove the last lines in doc/sphinx/source/conf.py in your branch.

[GunChleoc (gunchleoc)]

cppcheck would be nice, but only without failing the builds - the 3 memory leaks cited above are false positives. We'd have to completely redesign the memory handling in the Panel class to get rid of those warnings. Since it has proven to work fine across many years, I am very reluctant to invest development time into that one, because we have more important fish to fry.

I would love to get the full cppcheck as clean as reasonably possible though, because there is good potential for catching some bugs there.

[GunChleoc (gunchleoc)]

2 nits:

In cmake/codecheck/rules/camel_case_for_classes:

    "Use CapitalLetterWithCamelCase when naming an enum class, struct, or \"using\".")

 -> "Use CapitalLetterWithCamelCase when naming an enum class, class, struct, or \"using\".")

src/build_info.h

/// \return the build id which is automagically created from the checkout's

-> checkouts (plural, not genitive)

[SirVer (sirver)]

> I've made a website-branch to fix displaying of formulas when switching to mathjax:
> https://code.launchpad.net/~widelands-dev/widelands-website/mathjax_for_documentation

Thanks! I think mathjax is a better solution for typesetting formulas (I switched my personal blog also from images to mathjax). It transfers better to mobile and allows for better customizations by the client, i.e. the users viewing the page. It is also possible to decipher for see-impaired users.

> This works fine except for the coloring of formulas. As far i remember i added the coloring for better contrast, but my test with the above website-branch shows good contrast for the formulas. So i don't know if we want the coloring anymore. If not you could remove the last lines in doc/sphinx/source/conf.py in your branch.

I removed the lines.

> -> checkouts (plural, not genitive)

I actually think this should be the genitive. But I just removed the word "revision number or the VERSION file."

[GunChleoc (gunchleoc)]

You're right, I overlooked that the comment continues in the following line.

@bunnybot merge

review: Approve

=== modified file '.travis.sh'
--- .travis.sh	2017-06-27 16:45:47 +0000
+++ .travis.sh	2017-07-04 19:18:17 +0000
@@ -34,6 +34,7 @@
    libsdl2-image-dev \
    libsdl2-mixer-dev \
    libsdl2-ttf-dev \
+   python-pip \
    zlib1g-dev \
 ; do sleep 10; done
 
@@ -42,8 +43,22 @@
 cd build
 cmake .. -DCMAKE_BUILD_TYPE:STRING="$BUILD_TYPE"
 
-# Any codecheck warning is an error in Debug builds. Keep the codebase clean!!
 if [ "$BUILD_TYPE" == "Debug" ]; then
+
+   # Build the documentation. Any warning is an error.
+   sudo pip install sphinx
+   pushd ../doc/sphinx
+   mkdir source/_static
+   ./extract_rst.py
+   sphinx-build -W -b json -d build/doctrees source build/json
+   popd
+
+   # Run the codecheck test suite.
+   pushd ../cmake/codecheck
+   ./run_tests.py
+   popd
+
+   # Any codecheck warning is an error in Debug builds. Keep the codebase clean!!
    # Suppress color output.
    TERM=dumb make -j1 codecheck 2>&1 | tee codecheck.out
    if grep '^[/_.a-zA-Z]\+:[0-9]\+:' codecheck.out; then 

=== modified file 'cmake/codecheck/rules/camel_case_for_classes'
--- cmake/codecheck/rules/camel_case_for_classes	2014-09-18 15:53:08 +0000
+++ cmake/codecheck/rules/camel_case_for_classes	2017-07-04 19:18:17 +0000
@@ -4,39 +4,55 @@
 
 strip_comments_and_strings = True
 
+
 def evaluate_matches(lines, fn):
-   errors = []
-
-   for lineno, line in enumerate(lines):
-
-      # Exception for Microsoft fix in src/network/network_system.h
-      if line.count("in_port_t = uint16_t") or line.count("in_addr_t = uint32_t"):
-         continue
-
-      if re.match("(^\s*((class|struct)\s+((\S+_)|([a-z]\S+)).*{)|((enum)\s+((\S+_)|(([abd-z])|(c(?!lass))\S+)).*{)|((using)\s+((\S+_)|([a-z]\S+)).*=).*$)", line):
-
-         errors.append((fn, lineno+1, "Use CapitalLetterWithCamelCase when naming an enum class, struct, or \"using\"."))
-
-   return errors
+    errors = []
+
+    for lineno, line in enumerate(lines):
+
+        # Exception for Microsoft fix in src/network/network_system.h
+        if line.count('in_port_t = uint16_t') or line.count('in_addr_t = uint32_t'):
+            continue
+
+        words = line.strip().split()
+        if (len(words) < 2 or words[0] not in ['class', 'struct', 'enum', 'using']
+                or words[1] == '{'):
+            continue
+        if words[0] == 'using' and words[1] == 'namespace' or words[1].startswith('std::'):
+            continue
+        if words[0] == 'enum':
+            if words[1] == 'class':
+                words.pop(1)
+                if len(words) < 2:
+                    continue
+        if words[0] in ['class', 'struct', 'enum'] and words[-1] != '{':
+            continue
+        if re.match(r"([A-Z][a-z0-9]*)+", words[1]):
+            continue
+        errors.append(
+            (fn, lineno + 1,
+                "Use CapitalLetterWithCamelCase when naming an enum class, class, struct, or \"using\"."))
+
+    return errors
 # /end evaluate_matches
 
 forbidden = [
-    "class my_class",
-    "class myClass",
-    "struct my_struct",
-    "struct myStruct",
-    "using my_typedef",
-    "using myTypedef",
-    "enum class my_enum",
-    "enum class myEnum"
-    "enum my_enum",
-    "enum myEnum"
+    'class my_class {',
+    'class myClass {',
+    'struct my_struct {',
+    'struct myStruct {',
+    'using my_typedef {',
+    'using myTypedef {',
+    'enum class my_enum {',
+    'enum class myEnum {'
+    'enum my_enum {',
+    'enum myEnum {'
 ]
 
 allowed = [
-    "class MyClass",
-    "struct MyStruct",
-    "using MyTypedef",
-    "enum class MyEnum",
-    "enum MyEnum"
+    'class MyClass {',
+    'struct MyStruct {',
+    'using MyTypedef {',
+    'enum class MyEnum {',
+    'enum MyEnum {'
 ]

=== modified file 'cmake/codecheck/rules/missing_space_starting_comment'
--- cmake/codecheck/rules/missing_space_starting_comment	2016-02-18 18:22:10 +0000
+++ cmake/codecheck/rules/missing_space_starting_comment	2017-07-04 19:18:17 +0000
@@ -5,11 +5,36 @@
 Comments should all start with a blank space
 """
 
-error_msg = "Comments need to start with a space."
 
 strip_comments_and_strings = False
 regexp = r"""((^|\s)//[^\s/<!])|((^|\s)//<[^\s])|((^|\s)//!<[^\s])"""
 
+def is_space(s, index):
+    if index >= len(s):
+        return True
+    return s[index] == " "
+
+def evaluate_matches(lines, fn):
+   errors = []
+   for lineno, line in enumerate(lines):
+      line = line.strip()
+      if not line.startswith('//'):
+          continue
+      if line.startswith('///!<') and is_space(line, 5):
+          continue
+      if line.startswith('//!<') and is_space(line, 4):
+          continue
+      if line.startswith('///<') and is_space(line, 4):
+          continue
+      if line.startswith('///') and is_space(line, 3):
+          continue
+      if line.startswith('//<') and is_space(line, 3):
+          continue
+      if line.startswith('//') and is_space(line, 2):
+          continue
+      errors.append((fn, lineno+1, "Comments need to start with a space."))
+   return errors
+
 
 forbidden = [
     '//spaceless comment',

=== removed file 'cmake/codecheck/rules/upcast_without_macro'
--- cmake/codecheck/rules/upcast_without_macro	2016-01-30 19:08:48 +0000
+++ cmake/codecheck/rules/upcast_without_macro	1970-01-01 00:00:00 +0000
@@ -1,32 +0,0 @@
-#!/usr/bin/python
-
-"""
-This catches upcasts like this: Some_Type * const an_identifier =
-dynamic_cast<Some_Type *>(source) This is unnecessarily verbose and should be
-abbreviated with the upcast macro: upcast(Some_Type, an_identifier, source)
-"""
-
-regexp=r"""(^(<?!#define)\s*|[^:][^_a-zA-Z0-9]|:[^: ]) *(([_a-zA-Z][_a-zA-Z0-9]* *::)* *[_a-zA-Z][_a-zA-Z0-9]*(( +const)? *\*)*) *(const *)?\* *const +[_a-zA-Z][_a-zA-Z0-9]* *= *dynamic_cast *< *\2 *(const *)?\* *>"""
-
-error_msg = "Your upcast is ugly. Use upcast() macro!"
-
-forbidden = [
-    '	B_t * const hej = dynamic_cast<B_t *>(du);',
-    'if (B_t const * const hej = dynamic_cast<B_t *>(du)) {',
-    'B_t const*const hej=dynamic_cast  < B_t* >(du)) {',
-    'const B_t *const hej=dynamic_cast<B_t*>',
-    'MN:B_t *const hej=dynamic_cast<B_t*>',
-    'MN::B_t *const hej=dynamic_cast<MN::B_t*>',
-    ' B_t const * const hej=dynamic_cast<B_t const*>',
-    'B_t const * const hej=dynamic_cast<B_t const*>',
-]
-
-
-allowed = [
-    'Some_Type ha = dynamic_cast<Some_Type>',
-    'Some_Type ha = dynamic_cast<Some_Typ>',
-    'const B_t * hej = dymamic_cast<B_t *>',
-    'MN::B_t *const hej=dynamic_cast<B_t*>',
-]
-
-

=== modified file 'cmake/codecheck/run_tests.py'
--- cmake/codecheck/run_tests.py	2012-06-08 22:19:16 +0000
+++ cmake/codecheck/run_tests.py	2017-07-04 19:18:17 +0000
@@ -2,6 +2,7 @@
 # encoding: utf-8
 #
 
+import sys
 import unittest
 
 from CodeCheck import CodeChecker, Preprocessor
@@ -53,7 +54,7 @@
         unittest.TestCase.__init__(self)
         self.before = before
         self.after = after
-        
+
     def runTest(self):
         preprocessor = Preprocessor()
         after = preprocessor.get_preprocessed_data("test", self.before, True, False)
@@ -61,7 +62,7 @@
             "Stripping comments from %r failed. Expected %r, got %r"
             % (self.before, self.after, after)
             )
-        
+
 comment_tests = [
     # Let's get the basics right.
     ("a b c",
@@ -97,5 +98,5 @@
 
     for before, after in comment_tests:
         suite.addTest(CommentsTest(before, after.splitlines(True)))
-
-    unittest.TextTestRunner(verbosity=1).run(suite)
+    success = unittest.TextTestRunner(verbosity=1).run(suite).wasSuccessful()
+    sys.exit(0 if success else 1)

=== modified file 'data/scripting/richtext.lua'
--- data/scripting/richtext.lua	2017-05-12 14:11:11 +0000
+++ data/scripting/richtext.lua	2017-07-04 19:18:17 +0000
@@ -12,6 +12,7 @@
 
 -- RST
 -- .. function:: localize_list(items, listtype, former_textdomain)
+--    :noindex:
 --
 --    Turns an array of string items into a localized string list with
 --    appropriate concatenation.
@@ -53,6 +54,7 @@
 
 -- RST
 -- .. function:: rt(text_or_attributes[, text = nil])
+--    :noindex:
 --
 --    Wraps a block of text into Lua rich text.
 --    Only call this once for the whole text that gets sent to the backend.
@@ -83,6 +85,7 @@
 
 -- RST
 -- .. function:: img(src[, attributes = nil])
+--    :noindex:
 --
 --    Turns an image src path into an image tag for richtext.
 --
@@ -113,6 +116,7 @@
 
 -- RST
 -- .. function:: h1(text_or_color[, text = nil])
+--    :noindex:
 --
 --    Returns a paragraph formatted as a big heading with a small gap after it.
 --    The mnemonic comes from HTML.
@@ -128,6 +132,7 @@
 
 -- RST
 -- .. function:: h2(text)
+--    :noindex:
 --
 --    Like :func:`h1` but smaller.
 --
@@ -138,6 +143,7 @@
 
 -- RST
 -- .. function:: h3(text)
+--    :noindex:
 --
 --    Like :func:`h2` but smaller.
 --
@@ -149,6 +155,7 @@
 
 -- RST
 -- .. function:: h4(text)
+--    :noindex:
 --
 --    Like :func:`h3` but smaller.
 --
@@ -160,6 +167,7 @@
 
 -- RST
 -- .. function:: p(text_or_attributes[, text = nil])
+--    :noindex:
 --
 --    Returns one paragraph with text followed by a small vertical gap. Options
 --    can be given as first argument similar to :func:`rt`.
@@ -177,7 +185,7 @@
 
 
 -- RST
--- .. function:: p(text_or_attributes[, text = nil])
+-- .. function:: p_font(text_or_attributes[, text = nil])
 --
 --    Returns one paragraph with text followed by a small vertical gap. Options
 --    can be given as first argument similar to :func:`rt`.
@@ -275,6 +283,7 @@
 
 -- RST
 -- .. function:: dl(dt, dd)
+--    :noindex:
 --
 -- This function imitates a HTML description list
 --    :arg dt: "description term", will be rendered in bold.
@@ -337,6 +346,7 @@
 
 -- RST
 -- .. function:: a(link)
+--    :noindex:
 --
 -- This function imitates a HTML link. We can't do real links yet, so the text just gets underlines.
 --    :arg link: the text to format
@@ -349,6 +359,7 @@
 
 -- RST
 -- .. function:: b(link)
+--    :noindex:
 --
 -- This makes the text bold.
 --    :arg link: the text to format
@@ -361,6 +372,7 @@
 
 -- RST
 -- .. function:: i(link)
+--    :noindex:
 --
 -- This makes the text italic.
 --    :arg link: the text to format

=== modified file 'doc/sphinx/source/conf.py'
--- doc/sphinx/source/conf.py	2016-08-07 08:27:55 +0000
+++ doc/sphinx/source/conf.py	2017-07-04 19:18:17 +0000
@@ -25,7 +25,7 @@
 
 # Add any Sphinx extension module names here, as strings. They can be extensions
 # coming with Sphinx (named 'sphinx.ext.*') or your custom ones.
-extensions = ['sphinx.ext.imgmath']
+extensions = ['sphinx.ext.mathjax']
 
 # Add any paths that contain templates here, relative to this directory.
 templates_path = ['_templates']
@@ -231,18 +231,3 @@
    u'The Widelands Development Team', 'Widelands', 'One line description of project.',
    'Miscellaneous'),
 ]
-
-# Documents to append as an appendix to all manuals.
-#texinfo_appendices = []
-
-# If false, no module index is generated.
-#texinfo_domain_indices = True
-
-# How to display URL addresses: 'footnote', 'no', or 'inline'.
-#texinfo_show_urls = 'footnote'
-
-# Options for displaying formulas
-# http://www.nongnu.org/dvipng/dvipng_4.html#Command_002dline-options
-# Use one of 'dvispnames' or 'svgnames' for coloring from
-# http://mirrors.ctan.org/macros/latex/contrib/xcolor/xcolor.pdf
-imgmath_dvipng_args = ['-gamma', '1.5', '-D', '110', '-bg', 'Transparent', '-fg', 'Cerulean']

=== modified file 'doc/sphinx/source/lua_tribes_workers.rst.org'
--- doc/sphinx/source/lua_tribes_workers.rst.org	2017-04-21 18:40:38 +0000
+++ doc/sphinx/source/lua_tribes_workers.rst.org	2017-07-04 19:18:17 +0000
@@ -73,7 +73,6 @@
       programs that define which type of space or resource the worker has to find
       on the map in order to do his work, and what that work is, including any
       animations and sounds played.
-      See :doc:`worker_program`.
 
 TODO(GunChleoc): create Worker Program Reference
 

=== modified file 'src/build_info.h'
--- src/build_info.h	2017-04-22 08:02:21 +0000
+++ src/build_info.h	2017-07-04 19:18:17 +0000
@@ -27,11 +27,11 @@
 // also need updating.
 constexpr uint16_t kWidelandsCopyrightEnd = 2017;
 
-///\return the build id which is automagically created from the checkout's
-/// revision number or the VERSION file
+/// \return the build id which is automagically created from the revision number
+/// or the VERSION file
 const std::string& build_id();
 
-///\return the build type, which is set during compile time (either manually
+/// \return the build type, which is set during compile time (either manually
 /// or to a default value)
 const std::string& build_type();
 

=== modified file 'src/io/filesystem/filesystem.h'
--- src/io/filesystem/filesystem.h	2017-01-25 18:55:59 +0000
+++ src/io/filesystem/filesystem.h	2017-07-04 19:18:17 +0000
@@ -135,7 +135,7 @@
 	FileSystem();
 
 	/// How to address the fs' topmost component (e.g. "" on Unix, "D:" on win32)
-	///\warning This is should \e not contain filesep!
+	/// \warning This is should \e not contain filesep!
 	std::string root_;
 
 #ifdef _WIN32

=== modified file 'src/logic/game.cc'
--- src/logic/game.cc	2017-06-25 08:20:25 +0000
+++ src/logic/game.cc	2017-07-04 19:18:17 +0000
@@ -720,7 +720,6 @@
 	   *new CmdChangeSoldierCapacity(get_gametime(), b.owner().player_number(), b, val));
 }
 
-/////////////////////// TESTING STUFF
 void Game::send_player_enemyflagaction(const Flag& flag,
                                        PlayerNumber const who_attacks,
                                        uint32_t const num_soldiers) {

=== modified file 'src/logic/playercommand.h'
--- src/logic/playercommand.h	2017-06-24 08:47:46 +0000
+++ src/logic/playercommand.h	2017-07-04 19:18:17 +0000
@@ -729,7 +729,6 @@
 	int32_t val;
 };
 
-/////////////TESTING STUFF
 struct CmdEnemyFlagAction : public PlayerCommand {
 	CmdEnemyFlagAction() : PlayerCommand(), serial(0), number(0) {
 	}  // For savegame loading

=== modified file 'src/profile/profile.cc'
--- src/profile/profile.cc	2017-01-25 18:55:59 +0000
+++ src/profile/profile.cc	2017-07-04 19:18:17 +0000
@@ -65,7 +65,7 @@
 
 Profile g_options(Profile::err_log);
 
-Section::Value::Value(const string& nname, const char* const nval) : used_(false), name_(nname) {
+Section::Value::Value(const std::string& nname, const char* const nval) : used_(false), name_(nname) {
 	set_string(nval);
 }
 

=== modified file 'src/profile/profile.h'
--- src/profile/profile.h	2017-06-25 19:12:30 +0000
+++ src/profile/profile.h	2017-07-04 19:18:17 +0000
@@ -57,9 +57,7 @@
 	friend class Profile;
 
 	struct Value {
-		using string = std::string;
-
-		Value(const string& name, const char* const value);
+		Value(const std::string& name, const char* const value);
 		Value(const Value&);
 		Value(Value&& other);
 
@@ -93,7 +91,7 @@
 
 	private:
 		bool used_;
-		string name_;
+		std::string name_;
 		std::unique_ptr<char[]> value_;
 
 		Value() = default;

=== modified file 'src/ui_basic/fullscreen_window.h'
--- src/ui_basic/fullscreen_window.h	2017-02-23 19:38:51 +0000
+++ src/ui_basic/fullscreen_window.h	2017-07-04 19:18:17 +0000
@@ -70,7 +70,7 @@
 	FullscreenWindow();
 	virtual ~FullscreenWindow();
 
-	///\return the size for texts fitting to current resolution
+	/// \return the size for texts fitting to current resolution
 	int fs_small();
 	int fs_big();
 

=== modified file 'src/ui_basic/slider.cc'
--- src/ui_basic/slider.cc	2017-05-21 18:15:17 +0000
+++ src/ui_basic/slider.cc	2017-07-04 19:18:17 +0000
@@ -348,10 +348,6 @@
 	Slider::layout();
 }
 
-////////////////////////////////////////////////////////////////////////////////
-//                               HORIZONTAL                                   //
-////////////////////////////////////////////////////////////////////////////////
-
 /**
  * \brief Redraw the slide bar. The horizontal bar is painted.
  *
@@ -422,10 +418,6 @@
 	Slider::layout();
 }
 
-////////////////////////////////////////////////////////////////////////////////
-//                               VERTICAL                                     //
-////////////////////////////////////////////////////////////////////////////////
-
 /**
  * \brief Redraw the slide bar. The vertical bar is painted.
  *
@@ -487,10 +479,6 @@
 		return false;
 }
 
-////////////////////////////////////////////////////////////////////////////////
-//                               DISCRETE                                     //
-////////////////////////////////////////////////////////////////////////////////
-
 DiscreteSlider::DiscreteSlider(Panel* const parent,
                                const int32_t x,
                                const int32_t y,