Skip to content

Commit

Permalink
implement custom cppcheck rules
Browse files Browse the repository at this point in the history
I recently noticed there were several invocations of `wcwidth()` that should
have been `fish_wcwidth()`. This adds custom cppcheck rules to detect that
mistake.
  • Loading branch information
Kurtis Rader committed Jun 18, 2016
1 parent e6d4ac5 commit dc58edd
Show file tree
Hide file tree
Showing 3 changed files with 21 additions and 1 deletion.
18 changes: 18 additions & 0 deletions .cppcheck.rule
@@ -0,0 +1,18 @@
<?xml version="1.0"?>
<rule version="1">
<pattern> wcwidth \(</pattern>
<message>
<id>wcwidthForbidden</id>
<severity>warning</severity>
<summary>Always use fish_wcwidth rather than wcwidth.</summary>
</message>
</rule>

<rule version="1">
<pattern> wcswidth \(</pattern>
<message>
<id>wcswidthForbidden</id>
<severity>warning</severity>
<summary>Always use fish_wcswidth rather than wcswidth.</summary>
</message>
</rule>
2 changes: 2 additions & 0 deletions CONTRIBUTING.md
Expand Up @@ -30,6 +30,8 @@ Ultimately we want lint free code. However, at the moment a lot of cleanup is re


To make linting the code easy there are two make targets: `lint` and `lint-all`. The latter does just what the name implies. The former will lint any modified but not committed `*.cpp` files. If there is no uncommitted work it will lint the files in the most recent commit. To make linting the code easy there are two make targets: `lint` and `lint-all`. The latter does just what the name implies. The former will lint any modified but not committed `*.cpp` files. If there is no uncommitted work it will lint the files in the most recent commit.


Fish has custom cppcheck rules in the file `.cppcheck.rule`. These help catch mistakes such as using `wcwidth()` rather than `fish_wcwidth()`. Please add a new rule if you find similar mistakes being made.

### Dealing With Lint Warnings ### Dealing With Lint Warnings


You are strongly encouraged to address a lint warning by refactoring the code, changing variable names, or whatever action is implied by the warning. You are strongly encouraged to address a lint warning by refactoring the code, changing variable names, or whatever action is implied by the warning.
Expand Down
2 changes: 1 addition & 1 deletion build_tools/lint.fish
Expand Up @@ -92,7 +92,7 @@ if set -q c_files[1]
# The stderr to stdout redirection is because cppcheck, incorrectly IMHO, writes its # The stderr to stdout redirection is because cppcheck, incorrectly IMHO, writes its
# diagnostic messages to stderr. Anyone running this who wants to capture its output will # diagnostic messages to stderr. Anyone running this who wants to capture its output will
# expect those messages to be written to stdout. # expect those messages to be written to stdout.
cppcheck -q --verbose --std=posix --std=c11 --language=c++ --template "[{file}:{line}]: {severity} ({id}): {message}" --suppress=missingIncludeSystem --inline-suppr --enable=$cppchecks $cppcheck_args $c_files 2>&1 cppcheck -q --verbose --std=posix --std=c11 --language=c++ --template "[{file}:{line}]: {severity} ({id}): {message}" --suppress=missingIncludeSystem --inline-suppr --enable=$cppchecks --rule-file=.cppcheck.rule $cppcheck_args $c_files 2>&1

This comment has been minimized.

Copy link
@floam

floam Jun 18, 2016

Member

Also, I believe --std=c11 is not necessary here (or we should also add --std=c++11?)

C++11 and C11 are the defaults. The only non-default --std we need to add is posix.

This comment has been minimized.

Copy link
@krader1961

krader1961 Jun 18, 2016

Author Contributor

I don't now recall why I added --std=c11. I deliberately did not add --std=c++11 because the code isn't supposed to be using constructs legal in that standard but not earlier standards.

This comment has been minimized.

Copy link
@floam

floam Jun 18, 2016

Member

Well, --std=c++11 is in effect unless you use --std=c++03 AFAICT. But given that we use some C++11 constructs/behavior and seem to be on our way to outing ourselves as C++11 officially, I'd say it's preferable that we are getting "forward looking" feedback.

end end


if type -q oclint if type -q oclint
Expand Down

3 comments on commit dc58edd

@floam
Copy link
Member

@floam floam commented on dc58edd Jun 18, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we have two places with cppcheck commands in fish script? (d77c20b?)

@krader1961
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. I hadn't noticed the cppcheck.sh script when I embarked on writing the more ambitious lint.fish script. I'd say remove it but since @ridiculousfish just updated it presumably he finds it useful.

@floam
Copy link
Member

@floam floam commented on dc58edd Jun 18, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest if we don't remove it, we update that to use our preferences and call that from here. Could use argv to give it some further options specific to the Make usage - it may be useful for people integrating it with other tools. I know nothing is expecting our template or stderr getting bubkis.

Please sign in to comment.