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.

.cppcheck.rules

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

.cppcheck.suppressions

@@ -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

.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

CONTRIBUTING.md

@@ -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

bin/hdrs.c

@@ -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; }

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