widelands-dev team mailing list archive

[Hans Joachim Desserud <launchpad@xxxxxxxxxxxx>]

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?

Diff comments:

> === modified file '.travis.sh'
> --- .travis.sh	2017-06-27 16:45:47 +0000
> +++ .travis.sh	2017-07-03 13:52:43 +0000
> @@ -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

Saw your comment about the default sphinx being too old. I assume we attempt to use some features which weren't implemented at that point in time?

> +   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!!

Are these enforced, i.e. do they break the build or do developers need to click on the build link and read the logs to see if new warnings were introduced?

>     # 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-03 13:52:43 +0000
> @@ -13,30 +13,43 @@
>        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\"."))
> +      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, struct, or \"using\"."))
>  
>     return errors
>  # /end evaluate_matches
>  
>  forbidden = [

Seems, like it didn't autoconvert, try the longform-url https://bugs.launchpad.net/widelands/+bug/1388228

> -    "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 {"
>  ]


-- 
https://code.launchpad.net/~widelands-dev/widelands/codecheck/+merge/326647
Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/codecheck into lp:widelands.