Permalink
Browse files

Implement `bin/style` and `bin/lint` scripts

This implements the commands documented in a previous update to the
CONTRIBUTING.md document. The commands are currently implemented as fish
scripts since they are derived from the equivalent scripts I wrote for
the Fish shell project. TODO is rewriting them as ksh scripts.
  • Loading branch information...
krader1961 committed Jan 15, 2018
1 parent 077d1d5 commit 894231c3ec28b403a9ee1c5992d827781c45035d
Showing with 565 additions and 14 deletions.
  1. +26 −0 .cppcheck.rules
  2. +10 −0 .cppcheck.suppressions
  3. +95 −0 .oclint
  4. +17 −14 CONTRIBUTING.md
  5. +4 −0 bin/hdrs.c
  6. +170 −0 bin/lint
  7. +80 −0 bin/oclint-json
  8. +50 −0 bin/partition_compile_db
  9. +113 −0 bin/style
@@ -0,0 +1,26 @@
<?xml version="1.0"?>
<![CDATA[
<!-- Sadly we can't enable the following two rules since doing so causes false
positives in standard header files rather than just project specific
source files. If we can find a way to enable these rules by also
excluding system include files we should do so.
<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>
<--!>
]]>
@@ -0,0 +1,10 @@
// suppress all instances of varFuncNullUB: "Passing NULL after the last typed
// argument to a variadic function leads to undefined behaviour." That's
// because all the places we do this are valid and won't cause problems even
// on a ILP64 platform because we're careful about using NULL rather than 0.
varFuncNullUB
// Suppress the warning about unmatched suppressions. At the moment these
// warnings are emitted even when removing the suppression comment results in
// the warning being suppressed. In other words this unmatchedSuppression
// warnings are false positives.
unmatchedSuppression
95 .oclint
@@ -0,0 +1,95 @@
rules:
rule-configurations:
#
# This is the default value (as of the time I wrote this) but I'm making
# it explicit since it needs to agree with the value used by clang-format.
# Thus, if we ever change the style guide to allow longer or shorter lines
# this should be changed (as well as the corresponding .clang-format file).
#
- key: LONG_LINE
value: 100
#
# The default limit for the length of variable names is 20. Long names are
# problematic but twenty chars results in way too many errors. So increase
# the limit to something more reasonable.
#
- key: LONG_VARIABLE_NAME
value: 30
#
# This allows us to avoid peppering our code with inline comments such as
#
# scoped_lock locker(m_lock); //!OCLINT(side-effect)
#
# Specifically, this config key tells oclint that the named classes have
# RAII behavior so the local vars are actually used.
#
- key: RAII_CUSTOM_CLASSES
value: scoped_lock scoped_buffer_t builtin_commandline_scoped_transient_t scoped_push
# We're slightly more persmissive regarding the total number of lines in a
# function. Default is 50.
- key: LONG_METHOD
value: 60
# We're slightly more persmissive regarding the number of non-comment
# lines in a function. Default is 30.
- key: NCSS_METHOD
value: 40
# We're willing to allow slighly more linearly independent paths through a
# function. Most of our code has a lot of `switch` blocks or consecutive
# `if` tests that are straightforward to interpret but which increase this
# metric. Default is 10.
- key: CYCLOMATIC_COMPLEXITY
value: 14
# We're willing to allow slighly more execution paths through a function.
# Default is 200.
- key: NPATH_COMPLEXITY
value: 300
disable-rules:
#
# A few instances of "useless parentheses" errors are meaningful. Mostly
# in the context of the `return` statement. Unfortunately the vast
# majority would result in removing parentheses that decreases
# readability. So we're going to ignore this warning and rely on humans to
# notice when the parentheses are truly not needed.
#
# Also, some macro expansions, such as FD_SET(), trigger this warning and
# we don't want to suppress each of those individually.
#
- UselessParentheses
#
# OCLint wants variable names to be at least three characters in length.
# Which would be fine if it supported a reasonable set of exceptions
# (e.g., "i", "j", "k") and allowed adding additional exceptions to match
# conventions employed by a project. Since it doesn't, and thus generates
# a lot of really annoying warnings, we're going to disable this rule.
#
- ShortVariableName
#
# This rule flags perfectly reasonable conditions like `if (!some_condition)`
# and is therefore just noise. Disable this rule.
#
- InvertedLogic
#
# The idea behind the "double negative" rule is sound since constructs
# like "!!(var & flag)" should be written as "static_cast<bool>(var &
# flag)". Unfortunately this rule has way too many false positives;
# especially in the context of assert statements. So disable this rule.
#
- DoubleNegative
#
# Avoiding bitwise operators in a conditional is a good idea with one
# exception: testing whether a bit flag is set. Which happens to be the
# only time you'll see something like `if (j->flags & JOB_CONSTRUCTED)`
# in project source.
#
- BitwiseOperatorInConditional
#
# I don't think I've ever seen a case where assigning a value to a
# parameter inside the function body was unclear, let along dangerour or
# an error. This rule is therefore just noise. Disable this rule.
#
- ParameterReassignment
@@ -75,10 +75,12 @@ of cleanup is required to reach that goal. For now simply try to avoid
introducing new lint.
To make linting the code easy there is the `bin/lint` command. If you
pass it the magic string `all` it will lint all the code. If you pass
it a list of files it will lint those. If run with no arguments it will
lint any uncommitted source files. If there are no uncommitted files it
will lint the files in the most recent commit.
pass it the magic string `--all` it will lint all the *src/cmd/ksh93*
code. If you pass it a list of files it will lint those. The paths can be
directory names in which case all the source beneath that directory will
be linted. If run with no arguments it will lint any uncommitted source
files. If there are no uncommitted files it will lint the files in the
most recent commit.
### Dealing With Lint Warnings
@@ -105,8 +107,17 @@ on the topic.
## Ensuring Your Changes Conform to the Style Guides
The following sections discuss the specific rules for the style that
should be used when writing AST ksh code. To ensure your changes conform
to the style rules you simply need to run
should be used when writing AST ksh code.
To make restyling the code easy there is the `bin/style` command. If you
pass it the magic string `--all` it will restyle all the *src/cmd/ksh93*
code. If you pass it a list of files it will restyle those. The paths can be
directory names in which case all the source beneath that directory will
be restyled. If run with no arguments it will restyle any uncommitted source
files. If there are no uncommitted files it will restyle the files in the
most recent commit.
To ensure your changes conform to the style rules you simply need to run
```sh
bin/style
@@ -122,14 +133,6 @@ is correct. However, in that case it will run `clang-format` to ensure
the entire file, not just the lines modified by the commit, conform to
the style.
If you want to check the style of the entire code base run
```sh
bin/style all
```
That command will refuse to restyle any files if you have uncommitted changes.
### Configuring Your Editor
#### ViM
@@ -0,0 +1,4 @@
// This is a trivial program used by the `lint` script to find out where the
// compiler thinks the system headers live.
#include <stdlib.h>
int main() { return 0; }
170 bin/lint
@@ -0,0 +1,170 @@
#!/usr/bin/env fish
#
# TODO: Rewrite this as a ksh script.
#
# Usage: bin/lint [--all] [directory_or_filename]...
#
# Run the source through various lint detection tools. If invoked with `-all` then all the
# src/cmd/ksh93 source files will be linted. If invoked with one or more path names they
# will be linted. If the pathname is a directory all *.c files inside it will be linted.
# Otherwise any uncommitted source files are linted. If there is no uncommitted change
# then the files in the most recent commit are linted.
#
set all no
set cppchecks warning,performance,portability,information,missingInclude
set enable_global_analysis
set lint_args
set c_files
set files
set kernel_name (uname -s)
set machine_type (uname -m)
if not test -d build
or not test -f build/compile_commands.json
echo "You need to run `meson` to configure the build before we can lint the source." >&2
exit 1
end
# Deal with any CLI flags.
while set -q argv[1]
if test "$argv[1]" = "--all"
or test "$argv[1]" = "all"
set all yes
set -e argv[1]
else
break
end
end
if test $all = yes
and set -q argv[1]
echo "Unexpected arguments: '$argv'" >&2
exit 1
end
# Figure out which files to lint.
if test $all = yes
set files src/cmd/ksh93/**.c
else if set -q argv[1]
set files
for f in $argv
if test -f $argv
set files $files $f
else if test -d $argv
set files $files {$argv}**.c
end
end
else
# We haven't been asked to lint all the source or specific files. If there are uncommitted
# changes lint those, else lint the files in the most recent commit.
# Select (cached files) (modified but not cached, and untracked files).
set files (git diff-index --cached HEAD --name-only)
set files $files (git ls-files --exclude-standard --others --modified)
if not set -q files[1]
# No pending changes so lint the files in the most recent commit.
set files (git diff-tree --no-commit-id --name-only -r HEAD)
end
end
# Filter out non C source files.
set c_files
for file in (string match -r '^.*\.c$' -- $files)
if test -f $file
set c_files $c_files ../$file
end
end
cd build
# We need to limit the source modules to those for which we have build rules. We also need to
# produce a version of the compile_commands.json file that only contains the files to be linted.
# Finally, we need the `-D` and `-I` flags from the build rule for the IWYU and cppcheck programs.
set project_file compile_commands_partial.json
set defs_file cppcheck_defs
set c_files (../bin/partition_compile_db compile_commands.json $project_file $defs_file \
$c_files)
if not set -q c_files[1]
echo >&2
echo 'WARNING: No C files to check' >&2
echo >&2
exit 1
end
# On some platforms (e.g., macOS) oclint can't find the system headers. So ask the real compiler
# to tell us where they are and pass that information to oclint.
#
# Passing this path via the compiler `-isystem` flag also keeps oclint from complaining about
# problems with the system headers.
#
# We also need this value for cppcheck to find some system headers again, on platforms like macOS,
# where the system headers aren't found at /usr/include.
set system_hdrs (clang -H -E ../bin/hdrs.c 2>&1 | head -1 | sed -e 's/^\. //' -e 's/\/[^/]*$//')
# On macOS the system headers used by `clang` may not be rooted at /usr/include.
set lint_args $lint_args -I$system_hdrs
# This is needed with clang on macOS. Without it `cppcheck` fails with
# `#error Unsupported architecture` from `#include <sys/cdefs.h>`.
if test "$machine_type" = "x86_64"
set lint_args $lint_args -D__x86_64__ -D__LP64__
end
set lint_args $lint_args (cat $defs_file)
if type -q include-what-you-use
echo
echo ========================================
echo Running IWYU
echo ========================================
for c_file in $c_files
include-what-you-use -Xiwyu --transitive_includes_only --std=c99 \
-Wno-expansion-to-defined -Wno-nullability-completeness \
$lint_args $c_file 2>&1
end
end
if type -q cppcheck
echo
echo ========================================
echo Running cppcheck
echo ========================================
# 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
# expect those messages to be written to stdout.
set -l cn (set_color normal | string trim --chars \cO)
set -l cb (set_color --bold)
set -l cu (set_color --underline)
set -l cm (set_color magenta)
set -l cbrm (set_color brmagenta)
set -l template "[$cb$cu{file}$cn$cb:{line}$cn] $cbrm{severity}$cm ({id}):$cn\n {message}"
# It should be possible to use --project=$project_file but cppcheck 1.82 doesn't correctly
# extract the -D and -I flags. So we do it ourselves and pass the flags on the cppcheck
# command line.
set cppcheck_args $lint_args -q --verbose --std=c99 --std=posix --language=c \
--template $template --suppress=missingIncludeSystem --inline-suppr --enable=$cppchecks \
--rule-file=../.cppcheck.rules --suppressions-list=../.cppcheck.suppressions
cppcheck $cppcheck_args $c_files 2>&1
echo
echo ========================================
echo 'Running `cppcheck --check-config` to identify missing includes and similar problems.'
echo 'Ignore unmatchedSuppression warnings as they are probably false positives we'
echo 'cannot suppress.'
echo ========================================
cppcheck $cppcheck_args --check-config $c_files 2>&1
end
if type -q oclint
echo
echo ========================================
echo Running oclint
echo ========================================
# A copy of this config file has to be in the CWD (the Meson build dir).
test -f .ocling
or cp ../.oclint .
oclint -p $PWD -enable-clang-static-analyzer $enable_global_analysis \
-extra-arg="-isystem" -extra-arg="$system_hdrs" $c_files
end
Oops, something went wrong.

0 comments on commit 894231c

Please sign in to comment.