From 5cee5d7d2acf7b4cd8f98c2c50a12a1903cbf7b0 Mon Sep 17 00:00:00 2001 From: Michael Richters Date: Fri, 8 Apr 2022 21:37:22 -0500 Subject: [PATCH 01/10] Use `Runtime.*` instead of `Kaleidoscope.*` in SimHarness.cpp This means we can just include `Runtime.h` instead of all of `Kaleidoscope.h`, as a way of narrowing header includes. Signed-off-by: Michael Richters --- testing/SimHarness.cpp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/testing/SimHarness.cpp b/testing/SimHarness.cpp index 26f94f68..b6acb7e1 100644 --- a/testing/SimHarness.cpp +++ b/testing/SimHarness.cpp @@ -31,7 +31,7 @@ void SimHarness::RunCycle() { millis(); } } - Kaleidoscope.loop(); + kaleidoscope::Runtime.loop(); } void SimHarness::RunCycles(size_t n) { @@ -39,20 +39,20 @@ void SimHarness::RunCycles(size_t n) { } void SimHarness::RunForMillis(size_t t) { - auto start_time = Kaleidoscope.millisAtCycleStart(); - while (Kaleidoscope.millisAtCycleStart() - start_time < t) { + auto start_time = kaleidoscope::Runtime.millisAtCycleStart(); + while (kaleidoscope::Runtime.millisAtCycleStart() - start_time < t) { RunCycle(); } } void SimHarness::Press(KeyAddr key_addr) { - Kaleidoscope.device().keyScanner().setKeystate( + kaleidoscope::Runtime.device().keyScanner().setKeystate( key_addr, kaleidoscope::Device::Props::KeyScanner::KeyState::Pressed); } void SimHarness::Release(KeyAddr key_addr) { - Kaleidoscope.device().keyScanner().setKeystate( + kaleidoscope::Runtime.device().keyScanner().setKeystate( key_addr, kaleidoscope::Device::Props::KeyScanner::KeyState::NotPressed); } From 5571c13c790625d930404316ed75a821631eaa4a Mon Sep 17 00:00:00 2001 From: Michael Richters Date: Thu, 7 Apr 2022 13:59:18 -0500 Subject: [PATCH 02/10] Add missing newline at end of `.iwyu_ignore` file Signed-off-by: Michael Richters --- .iwyu_ignore | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.iwyu_ignore b/.iwyu_ignore index 576b8eb9..ce72373e 100644 --- a/.iwyu_ignore +++ b/.iwyu_ignore @@ -9,4 +9,4 @@ src/kaleidoscope/driver/mcu/GD32.h src/kaleidoscope/driver/storage/GD32Flash.h plugins/Kaleidoscope-FirmwareDump/** plugins/Kaleidoscope-HostOS/src/kaleidoscope/plugin/HostOS.h -plugins/Kaleidoscope-Hardware-EZ-ErgoDox/src/kaleidoscope/device/ez/ErgoDox/i2cmaster.h \ No newline at end of file +plugins/Kaleidoscope-Hardware-EZ-ErgoDox/src/kaleidoscope/device/ez/ErgoDox/i2cmaster.h From 4af5d6fc46994441ec1698b7baf7e3536c7e218b Mon Sep 17 00:00:00 2001 From: Michael Richters Date: Thu, 7 Apr 2022 14:02:05 -0500 Subject: [PATCH 03/10] Ignore files from gtest and examples when running IWYU Signed-off-by: Michael Richters --- .iwyu_ignore | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.iwyu_ignore b/.iwyu_ignore index ce72373e..fb8c1302 100644 --- a/.iwyu_ignore +++ b/.iwyu_ignore @@ -1,3 +1,4 @@ +examples src/Kaleidoscope.h src/Kaleidoscope-LEDControl.h src/kaleidoscope/HIDTables.h @@ -10,3 +11,4 @@ src/kaleidoscope/driver/storage/GD32Flash.h plugins/Kaleidoscope-FirmwareDump/** plugins/Kaleidoscope-HostOS/src/kaleidoscope/plugin/HostOS.h plugins/Kaleidoscope-Hardware-EZ-ErgoDox/src/kaleidoscope/device/ez/ErgoDox/i2cmaster.h +testing/googletest From 21d78160f409eff7e013736e65326b665372a45a Mon Sep 17 00:00:00 2001 From: Michael Richters Date: Thu, 7 Apr 2022 14:03:47 -0500 Subject: [PATCH 04/10] Allow specification of additional include dirs on command line This allows `bin/iwyu.py` to get library dirs other than the ones for Kaleidoscope when running on files in `testing`, without risk of collisions when running on Kaleidoscope itself. Signed-off-by: Michael Richters --- bin/iwyu.py | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/bin/iwyu.py b/bin/iwyu.py index 10928b60..15cd81a2 100755 --- a/bin/iwyu.py +++ b/bin/iwyu.py @@ -112,6 +112,15 @@ def parse_args(args): action='store', default='.iwyu_ignore', ) + parser.add_argument( + '-I', + '--include', + dest='include_dirs', + metavar='', + help= + """Add to the list of directories that will be searched for header files.""", + action='append', + ) parser.add_argument( 'targets', metavar="", @@ -256,6 +265,8 @@ def main(): ] # Include plugin source dirs for plugins that depend on other plugins: includes += glob.glob(os.path.join(kaleidoscope_dir, 'plugins', '*', 'src')) + # Include dirs specified on the command line: + includes += map(os.path.abspath, opts.include_dirs) clang_opts += ['-I' + _ for _ in includes] # ---------------------------------------------------------------------- From a7e6f3f07258e02155f20be4068249b27b837f25 Mon Sep 17 00:00:00 2001 From: Michael Richters Date: Thu, 7 Apr 2022 14:04:10 -0500 Subject: [PATCH 05/10] When running IWYU, process source files before headers Suppose the following: `foo.cpp` refers to the symbol `Bar`, declared in `bar.h`. `foo.h` includes `bar.h`. `foo.cpp` includes `foo.h`, but not `bar.h`. `foo.h` does not refer to any symbols declared in `bar.h`. If we process `foo.h` first, `#include "bar.h"` will be removed, causing IWYU to fail with an error when it tries to process `foo.cpp`, but if we process them in the other order, `foo.cpp` will get that include before it gets removed from `foo.h`. This change sorts the files to be processed, putting all cpp files first, then all header files, minimizing that problem. We could do even better by saving the results from `include-what-you-use` for every file, then going back and calling `fix_includes.py` on each of them, but I don't think it's worth it. Signed-off-by: Michael Richters --- bin/fix-header-includes | 4 +++- bin/iwyu.py | 20 ++++++++++++++++---- 2 files changed, 19 insertions(+), 5 deletions(-) diff --git a/bin/fix-header-includes b/bin/fix-header-includes index c25d2706..13c105cb 100755 --- a/bin/fix-header-includes +++ b/bin/fix-header-includes @@ -3,4 +3,6 @@ : "${KALEIDOSCOPE_DIR:=$(pwd)}" cd "${KALEIDOSCOPE_DIR}" || exit 1 -git ls-files -m | grep -E '\.(h|cpp)$' | xargs "${KALEIDOSCOPE_DIR}"/bin/iwyu.py +# Process sources first, then headers to minimize errors from removed includes +git ls-files -m | grep -E '\.(cpp|ino)$' | xargs "${KALEIDOSCOPE_DIR}"/bin/iwyu.py +git ls-files -m | grep -E '\.h$' | xargs "${KALEIDOSCOPE_DIR}"/bin/iwyu.py diff --git a/bin/iwyu.py b/bin/iwyu.py index 15cd81a2..93c6372e 100755 --- a/bin/iwyu.py +++ b/bin/iwyu.py @@ -296,12 +296,24 @@ def main(): # ---------------------------------------------------------------------- regex = re.compile(opts.regex) + # Process source files first, then header files, because a source file might have been + # relying on a header included by its associated header, but which that header does not + # need on its own. In this case, if we process the header first, IWYU won't be able to + # parse the source file, and we'll get an error, but if we do them in the other order, + # it'll be fine. + source_files = [] + header_files = [] + for target_file in (_ for t in targets for _ in build_target_list(t, regex)): + if target_file.endswith('.cpp') or target_file.endswith('.ino'): + source_files.append(target_file) + else: + header_files.append(target_file) exit_code = 0 - for src in (_ for t in targets for _ in build_target_list(t, regex)): - if src in ignores: - logging.info("Skipping ignored file: %s", os.path.relpath(src)) + for target_file in source_files + header_files: + if target_file in ignores: + logging.info("Skipping ignored file: %s", os.path.relpath(target_file)) continue - if not run_iwyu(os.path.relpath(src), iwyu_cmd, fix_includes_cmd): + if not run_iwyu(os.path.relpath(target_file), iwyu_cmd, fix_includes_cmd): exit_code = 1 return exit_code From fa0bb377c75fd33c2e6f9e2967827ec9670c5131 Mon Sep 17 00:00:00 2001 From: Michael Richters Date: Mon, 11 Apr 2022 10:21:45 -0500 Subject: [PATCH 06/10] Rewrite iwyu.py and format-code.py with various improvements Signed-off-by: Michael Richters --- bin/common.py | 110 +++++++++++++++ bin/format-code.py | 332 +++++++++++++++++++++++---------------------- bin/iwyu.py | 248 ++++++++++++++++++++------------- 3 files changed, 433 insertions(+), 257 deletions(-) create mode 100755 bin/common.py diff --git a/bin/common.py b/bin/common.py new file mode 100755 index 00000000..a431080b --- /dev/null +++ b/bin/common.py @@ -0,0 +1,110 @@ +#!/usr/bin/env python3 +# -*- coding: utf-8 -*- +# ------------------------------------------------------------------------------ +# Copyright (c) 2022 Michael Richters + +# This is free and unencumbered software released into the public domain. + +# Anyone is free to copy, modify, publish, use, compile, sell, or +# distribute this software, either in source code form or as a compiled +# binary, for any purpose, commercial or non-commercial, and by any +# means. + +# In jurisdictions that recognize copyright laws, the author or authors +# of this software dedicate any and all copyright interest in the +# software to the public domain. We make this dedication for the benefit +# of the public at large and to the detriment of our heirs and +# successors. We intend this dedication to be an overt act of +# relinquishment in perpetuity of all present and future rights to this +# software under copyright law. + +# THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, +# EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF +# MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. +# IN NO EVENT SHALL THE AUTHORS BE LIABLE FOR ANY CLAIM, DAMAGES OR +# OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, +# ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR +# OTHER DEALINGS IN THE SOFTWARE. + +# For more information, please refer to +# ------------------------------------------------------------------------------ +"""Utilities shared by Kaleidoscope code maintenance tools.""" + +import logging +import os +import subprocess +import sys + +from contextlib import contextmanager + + +# ============================================================================== +@contextmanager +def cwd(temp_wd): + """Execute code in a different working directory + + Parameters: + `temp_wd` (`str`): The name of a directory to (temporarily) change to + + Change the current working directory, then automatically restore the previous working + directory when done. Invoke `cwd()` like this: + ```py + with cwd(temp_wd): + ... + ``` + """ + old_wd = os.getcwd() + os.chdir(temp_wd) + try: + yield + finally: + os.chdir(old_wd) + + +# ============================================================================== +def split_on_newlines(string): + """Split the string using newlines as the separator.""" + return string.splitlines() + + +# ------------------------------------------------------------------------------ +def split_on_nulls(string): + """Split the input string using NULL characters as the separator.""" + targets = [_ for _ in string.split('\0') if _ != ''] + return targets or [] + + +# ============================================================================== +def setup_logging(loglevel): + """Set up basic logging.""" + logformat = "%(message)s" + logging.basicConfig( + level=loglevel, + stream=sys.stdout, + format=logformat, + datefmt="", + ) + return logging.getLogger() + + +# ============================================================================== +def check_git_diff(): + """Check for unstaged changes with `git diff` + + Returns: a list of the names of files with unstaged changes + + This check isn't perfect, because it can give false positives (if there are unrelated + unstaged changes). + """ + git_diff_cmd = ['git', 'diff', '-z', '--exit-code', '--name-only'] + + changed_files = [] + proc = subprocess.run(git_diff_cmd, capture_output=True) + if proc.returncode != 0: + changed_files = split_on_nulls(proc.stdout.decode('utf-8')) + return changed_files + + +# ============================================================================== +if __name__ == "__main__": + sys.exit(1) diff --git a/bin/format-code.py b/bin/format-code.py index a71c19ba..59a464d4 100755 --- a/bin/format-code.py +++ b/bin/format-code.py @@ -28,23 +28,27 @@ # For more information, please refer to # ------------------------------------------------------------------------------ -"""This script runs clang-format on Kaleidoscope's codebase.""" +"""This script runs clang-format on a Kaleidoscope repository.""" import argparse -import glob import logging import os import re +import shutil import subprocess import sys +sys.dont_write_bytecode = True + +from common import check_git_diff, setup_logging, split_on_newlines, split_on_nulls + # ============================================================================== def parse_args(args): """Parse command line parameters Args: - args (List[str]): command line parameters as list of strings + args (list[str]): command line parameters as list of strings (for example ``["--help"]``). Returns: @@ -55,217 +59,215 @@ def parse_args(args): Recursively search specified directories and format source files with clang-format. By default, it operates on Arduino C++ source files with extensions: *.{cpp,h,hpp,inc,ino}.""") + parser.add_argument( + '-q', + '--quiet', + dest='loglevel', + action='store_const', + const=logging.ERROR, + default=logging.WARNING, + help=""" + Suppress output except warnings and errors.""", + ) parser.add_argument( '-v', '--verbose', - dest='loglevel', - help="Verbose output", action='store_const', + dest='loglevel', const=logging.INFO, + help=""" + Output verbose debugging information.""", ) parser.add_argument( - '-q', - '--quiet', - dest='loglevel', - help="Suppress all non-error output", + '-d', + '--debug', action='store_const', - const=logging.ERROR, + dest='loglevel', + const=logging.DEBUG, + help=""" + Save output from `include-what-you-use` for processed files beside the originals, with + a '.iwyu' suffix, for debugging purposes.""", ) parser.add_argument( '-X', '--exclude-dir', - metavar="", - dest='exclude_dirs', - help="Exclude dir from search (path relative to the pwd)", action='append', + dest='exclude_dirs', default=[], + metavar="", + help=""" + Exclude dir from search (path relative to the pwd)""", ) parser.add_argument( '-x', '--exclude-file', - metavar="", - dest='exclude_files', - help="Exclude (base name only, not a full path) from formatting", action='append', + dest='exclude_files', default=[], + metavar="", + help=""" + Exclude (base name only, not a full path) from formatting""", ) parser.add_argument( - '-e', + '-r', '--regex', - metavar="", - dest='src_re_str', - help="Regular expression for matching source file names", + dest='regex', default=r'\.(cpp|h|hpp|inc|ino)$', + metavar="", + help=""" + Regular expression for matching source file names""", + ) + parser.add_argument( + '-z', + '-0', + action='store_const', + dest='input_splitter', + const=split_on_nulls, + default=split_on_newlines, + help=""" + When reading target filenames from standard input, break on NULL characters instead + of newlines.""", ) parser.add_argument( '-f', '--force', action='store_true', - help="Format code even if there are unstaged changes", + help=""" + Format code even if there are unstaged changes""", ) parser.add_argument( '--check', action='store_true', - help="Check for changes after formatting", + help=""" + Check for changes after formatting by running `git diff --exit-code`. If there are any + changes after formatting, a non-zero exit code is returned.""", ) parser.add_argument( 'targets', metavar="", - nargs='+', - help="""A list of files and/or directories to search for source files to format""", + nargs='*', + help=""" + A list of files and/or directories to search for source files to format.""", ) return parser.parse_args(args) # ============================================================================== -def setup_logging(loglevel): - """Setup basic logging - - Args: - loglevel (int): minimum loglevel for emitting messages +def main(): + """Parse command-line arguments and format source files. """ - logformat = "%(message)s" - logging.basicConfig( - level=loglevel, - stream=sys.stdout, - format=logformat, - datefmt="", - ) - return logging.getLogger() - - -# ============================================================================== -def format_code(path, opts, clang_format_cmd): - """Run clang-format on a directory.""" - logging.info("Formatting code in %s...", path) - - src_regex = re.compile(opts.src_re_str) - - src_files = [] - - for root, dirs, files in os.walk(path): - for exclude_path in opts.exclude_dirs: - exclude_path = exclude_path.rstrip(os.path.sep) - if os.path.dirname(exclude_path) == root: - exclude_dir = os.path.basename(exclude_path) - if exclude_dir in dirs: - dirs.remove(exclude_dir) - - for name in files: - if name in opts.exclude_files: - continue - if src_regex.search(name): - src_files.append(os.path.join(root, name)) - - proc = subprocess.run(clang_format_cmd + src_files) + # Parse command-line argumets: + opts = parse_args(sys.argv[1:]) + # Set up logging system: + setup_logging(opts.loglevel) + + # ---------------------------------------------------------------------- + # Unless we've been given the `--force` option, check for unstaged changes to avoid + # clobbering any work in progress: + exit_code = 0 + if not opts.force: + changed_files = check_git_diff() + if len(changed_files) > 0: + logging.error("Working tree has unstaged changes; aborting") + return 1 + + # Locate `clang-format` executable: + clang_format_exe = shutil.which('clang-format') + logging.debug("Found `clang-format` executable: %s", clang_format_exe) + clang_format_cmd = [clang_format_exe, '-i'] + if opts.loglevel <= logging.INFO: + clang_format_cmd.append('--verbose') + + # ---------------------------------------------------------------------- + # Read targets from command line: + targets = opts.targets + logging.debug("CLI target parameters: %s", targets) + + # If stdin is a pipe, read target filenames from it: + if not sys.stdin.isatty(): + targets += opts.input_splitter(sys.stdin.read()) + logging.debug("All targets: %s", targets) + + # Prepare exclusion lists. The file excludes are basenames only, and the dirs get + # converted to absolute path names. + exclude_files = set(opts.exclude_files) + exclude_dirs = set(os.path.abspath(_) for _ in opts.exclude_dirs) + + # Convert target paths to absolute, and remove any that are excluded: + target_paths = set(os.path.abspath(_) for _ in targets if _ not in exclude_dirs) + logging.debug("Target paths: %s", target_paths) + + # Build separate sets of target files and dirs. Later, we'll search target dirs and add + # matching target files to the files set. + target_files = set() + target_dirs = set() + for t in target_paths: + if os.path.isfile(t): + target_files.add(os.path.abspath(t)) + elif os.path.isdir(t): + target_dirs.add(os.path.abspath(t)) + logging.debug("Target files after separating: %s", target_files) + logging.debug("Target dirs after separating: %s", target_dirs) + + # Remove excluded filenames: + target_files -= set(_ for _ in target_files if os.path.basename(_) in exclude_files) + + # Remove files and dirs in excluded dirs: + target_files -= set(_ for _ in target_files for x in exclude_dirs if _.startswith(x)) + target_dirs -= set(_ for _ in target_dirs for x in exclude_dirs if _.startswith(x)) + + # Compile regex for matching files to be formatted: + target_matcher = re.compile(opts.regex) + + # Remove target files that don't match the regex: + logging.debug("Target files before matching regex: %s", target_files) + target_files = set(_ for _ in target_files if target_matcher.search(_)) + logging.debug("Target files after matching regex: %s", target_files) + + # Search target dirs for non-excluded files, and add them to `target_files`: + logging.debug("Searching target dirs: %s", target_dirs) + for path in target_dirs: + for root, dirs, files in os.walk(path): + # Prune excluded dirs + for x in exclude_dirs: + if x in (os.path.join(root, _) for _ in dirs): + dirs.remove(os.path.basename(x)) + # Add non-excluded files + for f in files: + if target_matcher.search(f) and f not in exclude_files: + target_files.add(os.path.join(root, f)) + + if len(target_files) == 0: + logging.error("No target files found; exiting.") + return 1 + + # Run clang-format on target files: + proc = subprocess.run(clang_format_cmd + sorted(target_files)) if proc.returncode != 0: logging.error("Error: clang-format returned non-zero status: %s", proc.returncode) - return - - -# ============================================================================== -def build_file_list(path, src_regex): - """Docstring""" - - # If the specified path is a filename, return it (as a list), regardless of - # whether or not it matches the regex. - if os.path.isfile(path): - return [path] - - # If the specified path is not valid, just return an empty list. - if not os.path.isdir(path): - return [] - - # The specified path is a directory, so we search recursively for files - # contained therein that match the specified regular expression. - source_files = [] - for root, dirs, files in os.walk(path): - # First, ignore all dotfiles (and directories). - dotfiles = set(glob.glob('.*')) - dirs = set(dirs) - dotfiles - files = set(dirs) - dotfiles - - # Check for a list of file glob patterns that should be excluded. - if IWYU_IGNORE_FILE in files: - with open(os.path.join(root, IWYU_IGNORE_FILE)) as f: - for pattern in f.read().splitlines(): - matches = set(glob.glob(os.path.join(root, pattern))) - dirs = set(dirs) - matches - files = set(files) - matches - - # Add all matching files to the list of source files to be formatted. - for f in filter(src_regex.search, files): - source_files.append(os.path.join(root, f)) - - return source_files + return proc.returncode + else: + logging.info("Finished formatting target files.") - -# ============================================================================== -def warn_if_output(byte_str, msg): - """Convert a string of bytes to a UTF-8 string, break it on newlines. If - there is any output, print a warning message, followed by each line, - indented by four spaces.""" - lines = byte_str.decode('utf-8').splitlines() - if '' in lines: - lines.remove('') - if len(lines) > 0: - logging.warning('%s', msg) - for line in lines: - logging.warning(' %s', line) - return - - -# ============================================================================== -def main(cli_args): - """Parse command-line arguments and format source files.""" - args = parse_args(cli_args) - if args.loglevel is None: - args.loglevel = logging.WARNING - setup_logging(args.loglevel) - - clang_format = os.getenv('CLANG_FORMAT_CMD') - if clang_format is None: - clang_format = 'clang-format' - - clang_format_cmd = [clang_format, '-i'] - - git_diff_cmd = ['git', 'diff', '--exit-code'] - - proc = subprocess.run(git_diff_cmd + ['--name-only'], capture_output=True) - if proc.returncode != 0: - warn_if_output(proc.stdout, 'Warning: you have unstaged changes to these files:') - if not args.force: - logging.warning( - 'Formatting aborted. Stage your changes or use --force to override.') - sys.exit(proc.returncode) - - if args.loglevel >= logging.WARNING: - git_diff_cmd.append('--quiet') - elif args.loglevel <= logging.INFO: - git_diff_cmd.append('--name-only') - - for path in args.targets: - format_code(path, args, clang_format_cmd) - - if args.check: + # If we've been asked to check for changes made by the formatter: + if opts.check: logging.warning('Checking for changes made by the formatter...') + changed_files = check_git_diff() + if len(changed_files) == 0: + logging.warning("No files changed. Congratulations!") + else: + logging.warning("Found files with changes after formatting:") + exit_code = 1 + for f in changed_files: + logging.warning(" %s", f) - proc = subprocess.run(git_diff_cmd + ['--cached'], capture_output=True) - if proc.returncode != 0: - logging.warning( - 'Warning: Your working tree has staged changes. ' + - 'Committed changes might not pass this check.') - warn_if_output(proc.stdout, 'The following files have unstaged changes:') - - proc = subprocess.run(git_diff_cmd, capture_output=True) - if proc.returncode != 0: - warn_if_output(proc.stdout, 'The following files have changes:') - logging.error( - 'Check failed: Please commit formatting changes before submitting.') - sys.exit(proc.returncode) - return + return exit_code # ============================================================================== if __name__ == "__main__": - main(sys.argv[1:]) + try: + sys.exit(main()) + except KeyboardInterrupt: + logging.info("Aborting") + sys.exit(1) diff --git a/bin/iwyu.py b/bin/iwyu.py index 93c6372e..403ebc40 100755 --- a/bin/iwyu.py +++ b/bin/iwyu.py @@ -28,7 +28,9 @@ # For more information, please refer to # ------------------------------------------------------------------------------ -"""This is a script for maintenance of the headers included in Kaleidoscope source +"""Run `include-what-you-use` on Kaleidoscope sources + +This is a script for maintenance of the headers included in Kaleidoscope source files. It is not currently possible to run this automatically on all Kaleidoscope source files, because of the peculiarities therein. It uses llvm/clang to determine which headers should be included in a given file, but @@ -55,12 +57,25 @@ import shutil import subprocess import sys +sys.dont_write_bytecode = True + +from common import cwd, setup_logging, split_on_newlines, split_on_nulls + # ============================================================================== def parse_args(args): + """Parse command line parameters + + Args: + args (list[str]): command line parameters as list of strings + (for example ``["--help"]``). + + Returns: + :obj:`argparse.Namespace`: command line parameters namespace + """ parser = argparse.ArgumentParser( - description= - """Run `include-what-you-use` on source files given as command-line arguments and/or read + description=""" + Run `include-what-you-use` on source files given as command-line arguments and/or read from standard input. When reading target filenames from standard input, they should be either absolute or relative to the current directory, and each line of input (minus the line-ending character(s) is treated as a filename.""") @@ -68,65 +83,87 @@ def parse_args(args): '-q', '--quiet', dest='loglevel', - help="Suppress output except warnings and errors.", action='store_const', const=logging.ERROR, default=logging.WARNING, + help=""" + Suppress output except warnings and errors.""", ) parser.add_argument( '-v', '--verbose', - dest='loglevel', - help="Output verbose debugging information.", action='store_const', + dest='loglevel', const=logging.INFO, + help=""" + Output verbose debugging information.""", ) parser.add_argument( '-d', '--debug', - dest='loglevel', - help="""Save output from `include-what-you-use` for processed files beside the - originals, with a '.iwyu' suffix, for debugging purposes.""", action='store_const', + dest='loglevel', const=logging.DEBUG, + help=""" + Save output from `include-what-you-use` for processed files beside the originals, with + a '.iwyu' suffix, for debugging purposes.""", ) parser.add_argument( '-r', '--regex', + action='store', dest='regex', - help="""A regular expression for matching filenames Only the basename of the file is + default=r'\.(h|cpp)$', + help=""" + A regular expression for matching filenames Only the basename of the file is matched, and the regex is only used when searching a directory for files to process, not on target filenames specified in arguments or read from standard input.""", - action='store', - default=r'\.(h|cpp)$', ) parser.add_argument( '-i', '--ignores_file', - dest='ignores_file', - metavar='', - help= - """The name of a file (relative to KALEIDOSCOPE_DIR) that contains a list of glob - patterns that will be ignored when a target directory is searched for filenames that - match .""", action='store', + dest='ignores_file', default='.iwyu_ignore', + metavar='', + help=""" + The name of a file (relative to KALEIDOSCOPE_DIR) that contains a list of glob patterns + that will be ignored when a target directory is searched for filenames that match + .""", ) parser.add_argument( '-I', '--include', + action='append', dest='include_dirs', metavar='', - help= - """Add to the list of directories that will be searched for header files.""", - action='append', + help=""" + Add to the list of directories that will be searched for header files.""", + ) + parser.add_argument( + '-z', + '-0', + action='store_const', + dest='input_splitter', + const=split_on_nulls, + default=split_on_newlines, + help=""" + When reading target filenames from standard input, break on NULL characters instead + of newlines.""", + ) + parser.add_argument( + '--update_comments', + action='store_true', + help=""" + Call `include-what-you-use` with `--update_comments` to always rewrite its 'for' + comments regarding which symbols are used.""", ) parser.add_argument( 'targets', + nargs='*', metavar="", - nargs='+', - help= - """A list of target files and/or directories to search for source files to format. Any + help=""" + A list of target files and/or directories to search for source files to format. Any target file will be processed, regardless of the filename. Any target directory will be recursively searched for files matching the regular expression given by --regex. Filenames and directories beginning with a '.' will always be excluded from the search, @@ -135,22 +172,6 @@ def parse_args(args): return parser.parse_args(args) -# ============================================================================== -def setup_logging(loglevel): - """Set up basic logging - - Args: - :int:loglevel: minimum loglevel for emitting messages - """ - logformat = "%(message)s" - logging.basicConfig( - level=loglevel, - stream=sys.stdout, - format=logformat, - datefmt="", - ) - - # ============================================================================== def main(): """Main entry point function.""" @@ -162,12 +183,13 @@ def main(): # ---------------------------------------------------------------------- # Find include-what-you-use: iwyu = shutil.which('include-what-you-use') - logging.info("Found `include-what-you-use` executable: %s", iwyu) + logging.debug("Found `include-what-you-use` executable: %s", iwyu) iwyu_opts = [ '--no_fwd_decls', # No forward declarations '--max_line_length=100', - '--update_comments', ] + if opts.update_comments: + iwyu_opts.append('--update_comments') # Prepend '-Xiwyu' to each `include-what-you-use` option: iwyu_opts = [_ for opt in iwyu_opts for _ in ('-Xiwyu', opt)] @@ -265,8 +287,9 @@ def main(): ] # Include plugin source dirs for plugins that depend on other plugins: includes += glob.glob(os.path.join(kaleidoscope_dir, 'plugins', '*', 'src')) - # Include dirs specified on the command line: - includes += map(os.path.abspath, opts.include_dirs) + # Include dirs specified on the command line, if any: + if opts.include_dirs: + includes += [os.path.abspath(_) for _ in opts.include_dirs] clang_opts += ['-I' + _ for _ in includes] # ---------------------------------------------------------------------- @@ -276,19 +299,23 @@ def main(): fix_includes_cmd = [ fix_includes, - '--update_comments', '--nosafe_headers', '--reorder', '--separate_project_includes=' + kaleidoscope_src_dir, # Does this help? ] + if opts.update_comments: + fix_includes_cmd.append('--update_comments') logging.debug("Using `fix_includes` command: %s", ' \\\n\t'.join(fix_includes_cmd)) # ---------------------------------------------------------------------- targets = opts.targets - # If stdin is a pipe, read pathname targets, one per line. This allows us to - # connect the output of `find` to our input conveniently: + # If stdin is a pipe, read pathname targets, one per line. This allows us to connect the + # output of `find` to our input conveniently: if not sys.stdin.isatty(): - targets += sys.stdin.read().splitlines() + logging.debug("Reading targets from STDIN...") + targets += opts.input_splitter(sys.stdin.read()) + for t in targets: + logging.debug(" Target path: %s", t) # ---------------------------------------------------------------------- iwyu_ignores_file = os.path.join(kaleidoscope_dir, opts.ignores_file) @@ -300,101 +327,138 @@ def main(): # relying on a header included by its associated header, but which that header does not # need on its own. In this case, if we process the header first, IWYU won't be able to # parse the source file, and we'll get an error, but if we do them in the other order, - # it'll be fine. + # the source file will get all the includes it needs before the header removes any of them. source_files = [] header_files = [] - for target_file in (_ for t in targets for _ in build_target_list(t, regex)): + for target_file in (_ for t in targets for _ in build_target_list(t, regex, ignores)): if target_file.endswith('.cpp') or target_file.endswith('.ino'): source_files.append(target_file) else: header_files.append(target_file) + + # If there's an error processing any file, return an error code. exit_code = 0 for target_file in source_files + header_files: - if target_file in ignores: - logging.info("Skipping ignored file: %s", os.path.relpath(target_file)) - continue + # Run IWYU and fix_headers: if not run_iwyu(os.path.relpath(target_file), iwyu_cmd, fix_includes_cmd): exit_code = 1 + return exit_code # ============================================================================== -def build_target_list(path, src_regex): - """Docstring""" - logging.debug("Searching target: %s", path) +def build_target_list(target, regex, ignores): + """Build the list of target files, starting from a file or directory. + + Parameters: + target (str): The name of the file or directory to search + regex (Pattern): A compiled regular expression to match target file names + ignores (list): A list of (absolute) file names to be excluded from the target list + Returns: + targets (list): A list of (absolute) file names to be processed + + Given a filename or directory (`path`), returns a list of target filenames to run IWYU + on. Target filenames will all be absolute, but `path` can be relative to the current + directory. All target filenames will match `regex`, and any items in `ignores` will be + excluded. If an element of the `ignores` list is a directory, all files and directories + beneath it will be excluded from the search. + """ + logging.debug("Searching target: %s", target) - # If the specified path is a filename, return it (as a list), regardless of - # whether or not it matches the regex. - if os.path.isfile(path): - return [path] + # Convert all paths to absolute. + root = os.path.abspath(target) + + # If `path` is a file, and it matches `regex`, add it to the target list. + if os.path.isfile(target) and regex.search(target) and root not in ignores: + return [root] # If the specified path is not valid, just return an empty list. - if not os.path.isdir(path): - logging.error("Error: File not found: %s", path) + if not os.path.isdir(target): + logging.error("Error: File not found: %s", target) return [] + # Start with an empty list. + targets = [] + # The specified path is a directory, so we search recursively for files # contained therein that match the specified regular expression. - targets = [] - for root, dirs, files in os.walk(os.path.abspath(path)): - logging.debug("Searching dir: %s", root) + for path, dirs, files in os.walk(root): + logging.debug("Searching dir: %s", os.path.relpath(path)) # First, ignore all dotfiles (and directories). - dotfiles = set(glob.glob('.*')) - dirs = set(dirs) - dotfiles - files = set(files) - dotfiles + for x in (os.path.basename(_) + for _ in ignores + glob.glob(os.path.join(path, '.*')) + if os.path.dirname(_) == path): + if x in dirs: + logging.info("Skipping ignored dir: %s", os.path.join(path, x)) + dirs.remove(x) + if x in files: + logging.info("Skipping ignored file: %s", os.path.join(path, x)) + files.remove(x) logging.debug("Files found: %s", ', '.join(files)) # Add all matching files to the list of source files to be formatted. - for f in filter(src_regex.search, files): - logging.debug("Source file found: %s", f) - targets.append(os.path.join(root, f)) + for f in (_ for _ in files if regex.search(_)): + t = os.path.join(path, f) + logging.debug("Source file found: %s", t) + targets.append(t) - return [os.path.abspath(_) for _ in targets] + return targets # ============================================================================== def build_ignores_list(ignores_file_path): + """Build a list of files and dirs to exclude from processing by IWYU + + Parameters: + ignores_file_path (str): The name of the file to read ignores globs from + Returns: + ignores_list (list): A list of (absolute) file names to exclude from processing + + Reads `ignores_file` and expands each line as a glob, returning a list of all the target + file names to be excluded from processing. Each path in the file is treated as a relative + pathname (unless it is already absolute), relative to the directory in which `ignores_file` + is located. + """ logging.debug("Searching for ignores file: %s", ignores_file_path) # If the ignores file doesn't exist, return an empty list: if not os.path.isfile(ignores_file_path): logging.debug("Ignores file not found") return [] + ignores_list = [] with open(ignores_file_path) as f: for line in f.read().splitlines(): - logging.debug("Ignoring files like: %s", line) + # Lines starting with `#` are treated as comments. if line.startswith('#'): continue + logging.debug("Ignoring files like: %s", line) ignores_list += glob.glob(line, recursive=True) + + # Get the dir of the ignores file so we can construct absolute pathnames. ignores_file_dir = os.path.dirname(ignores_file_path) with cwd(ignores_file_dir): ignores_list[:] = [os.path.abspath(_) for _ in ignores_list] + logging.debug("Ignores list:\n\t%s", "\n\t".join(ignores_list)) return ignores_list -# ------------------------------------------------------------------------------ -from contextlib import contextmanager - - -@contextmanager -def cwd(path): - """A simple function change directory, an automatically restore the previous working - directory when done, using `with cwd(temp_dir):`""" - old_wd = os.getcwd() - os.chdir(path) - try: - yield - finally: - os.chdir(old_wd) - - # ============================================================================== def run_iwyu(source_file, iwyu_cmd, fix_includes_cmd): - """Run `include-what-you-use` on , an update that file's header includes by + """Run IWYU and fix_includes on a single source file + + Parameters: + source_file (str): The name of a file to run IWYU on + iwyu_cmd (list): The command name and options list for `include-what-you-use` + fix_includes_cmd (list): The command name and options list for `fix_headers.py` + Returns: + True on success, False if either IWYU or fix_headers returns an error code + + Run `include-what-you-use` on , an update that file's header includes by sending the output to `fix_includes.py`. If either command returns an error code, return - `False`, otherwise return `True`.""" - logging.info("Processing file: %s", source_file) + `False`, otherwise return `True`. + """ + logging.info("Fixing headers in file: %s", source_file) # Run IWYU on iwyu_proc = subprocess.run(iwyu_cmd + [source_file], capture_output=True, check=False) From ee33b228f90a70cd751b9b31ad4d6ec5004bd13f Mon Sep 17 00:00:00 2001 From: Michael Richters Date: Mon, 11 Apr 2022 10:22:36 -0500 Subject: [PATCH 07/10] Update Makefile and add wrapper script for managing header includes This adds a `bin/fix-header-includes` script that uses `iwyu.py` and `format-code.py` to manage header includes in Kaleidoscope source files. In addition to the `src` and `plugins` trees, it is now also capable of handling files in `testing` for the test simulator, which introduces some particular complications due to Arduino's ill-advised `min` and `max` preprocessor macros. The new `fix-header-includes` script can be run on a repository, and runs IWYU and clang-format on code that differs between the current worktree and a specified commit (default: `origin/master`), hopefully ensuring compliance with the code style guide. Also added: a new `check-all-includes` makefile target meant to be useful for running as a git workflows check. Signed-off-by: Michael Richters --- Makefile | 13 ++++++++++--- bin/fix-header-includes | 43 +++++++++++++++++++++++++++++++++++++---- 2 files changed, 49 insertions(+), 7 deletions(-) diff --git a/Makefile b/Makefile index 02062372..50bed699 100644 --- a/Makefile +++ b/Makefile @@ -113,21 +113,28 @@ check-code-style: bin/format-code.py \ --exclude-dir 'testing/googletest' \ --exclude-file 'generated-testcase.cpp' \ - --force \ --check \ --verbose \ src plugins examples testing .PHONY: check-includes check-includes: + bin/fix-header-includes + +.PHONY: check-all-includes +check-all-includes: bin/iwyu.py -v src plugins - bin/format-code.py -f -v --check src plugins + bin/iwyu.py -v \ + -I=$(KALEIDOSCOPE_DIR) \ + -I=$(KALEIDOSCOPE_DIR)/testing/googletest/googlemock/include \ + -I=$(KALEIDOSCOPE_DIR)/testing/googletest/googletest/include \ + testing + bin/format-code.py -f -v --check src plugins testing .PHONY: cpplint-noisy cpplint-noisy: -bin/cpplint.py --config=.cpplint-noisy --recursive src plugins examples - .PHONY: cpplint cpplint: bin/cpplint.py --config=.cpplint --quiet --recursive src plugins examples diff --git a/bin/fix-header-includes b/bin/fix-header-includes index 13c105cb..e44af80e 100755 --- a/bin/fix-header-includes +++ b/bin/fix-header-includes @@ -1,8 +1,43 @@ #!/usr/bin/env bash +# In case it hasn't been set, assume we're being run from the root of the local +# Kaleidoscope repository. : "${KALEIDOSCOPE_DIR:=$(pwd)}" -cd "${KALEIDOSCOPE_DIR}" || exit 1 -# Process sources first, then headers to minimize errors from removed includes -git ls-files -m | grep -E '\.(cpp|ino)$' | xargs "${KALEIDOSCOPE_DIR}"/bin/iwyu.py -git ls-files -m | grep -E '\.h$' | xargs "${KALEIDOSCOPE_DIR}"/bin/iwyu.py +# This variable is a git ref that should point to the current master branch of +# the primary Kaleidoscope repository. In most cases, this would be +# `origin/master`. +: "${KALEIDOSCOPE_MERGE_BASE:=origin/master}" + +# Don't do anything if the working tree has unstaged changes, to avoid +# unintentional combining of contentful changes with formatting changes. +if ! git diff -z --exit-code --quiet; then + echo "Working tree has unstaged changes; aborting." + exit 1 +fi + +# Run git-diff so we only run IWYU on files that differ from `master`. This +# isn't necessarily what we want, but if the current branch has been rebased, it +# shouldn't touch any extra files. +git diff -z --name-only "${KALEIDOSCOPE_MERGE_BASE}" -- src plugins \ + | "${KALEIDOSCOPE_DIR}/bin/iwyu.py" -z -v + +# After running it on Kaleidoscope source files, run it on the test simulator, +# which requires some additional include dirs. +git diff -z --name-only "${KALEIDOSCOPE_MERGE_BASE}" -- testing \ + | "${KALEIDOSCOPE_DIR}/bin/iwyu.py" \ + -z -v \ + -I="${KALEIDOSCOPE_DIR}" \ + -I="${KALEIDOSCOPE_DIR}/testing/googletest/googlemock/include" \ + -I="${KALEIDOSCOPE_DIR}/testing/googletest/googletest/include" + +# Always run clang-format after IWYU, because they have different indentation +# rules for comments added by IWYU. +git diff -z --name-only "${KALEIDOSCOPE_MERGE_BASE}" -- src plugins testing \ + | "${KALEIDOSCOPE_DIR}/bin/format-code.py" \ + -z -v \ + --exclude-dir='testing/googletest' \ + --exclude-file='generated-testcase.cpp' \ + --force \ + --check \ + --verbose From 9b04d6663ca5e0227063322dd6fd922e5a07e2ab Mon Sep 17 00:00:00 2001 From: Michael Richters Date: Mon, 11 Apr 2022 10:30:25 -0500 Subject: [PATCH 08/10] Run IWYU (with the new tools) on test simulator code This mixes some manual work (IWYU pragmas, a better solution to the Arduino preprocessor macros problem) with automated running of the tools. At this point, it would be too much work to separate these into distinct commits, and there isn't that much value to doing so. There are still some things we could do to make things more robust, as some of the headers need to be in a certain order, which happens to be in the same sort order used by IWYU (`testing/*` files need to come after certain headers than include `Arduino.h`), but it's probably not worth the clutter of adding an `#if 1` just to stop IWYU from re-ordering them. I tried to get `#pragma push_macro("max")/pop_macro("max")` to work, but ended up getting completely nonsensical compilation errors, so I gave up on it. Signed-off-by: Michael Richters --- testing/AbsoluteMouseReport.cpp | 9 ++++--- testing/AbsoluteMouseReport.h | 8 +++---- testing/ConsumerControlReport.cpp | 6 ++--- testing/ConsumerControlReport.h | 8 +++---- testing/ExpectedKeyboardReport.cpp | 3 +++ testing/ExpectedKeyboardReport.h | 8 +++---- testing/ExpectedMouseReport.cpp | 3 +++ testing/ExpectedMouseReport.h | 8 +++---- testing/HIDState.cpp | 10 ++++---- testing/HIDState.h | 21 +++++++++------- testing/KeyboardReport.cpp | 7 +++--- testing/KeyboardReport.h | 8 +++---- testing/MouseReport.cpp | 7 ++---- testing/MouseReport.h | 8 +++---- testing/SimHarness.cpp | 6 +++-- testing/SimHarness.h | 8 +++---- testing/State.h | 10 +++----- testing/SystemControlReport.cpp | 5 ++-- testing/SystemControlReport.h | 8 +++---- testing/VirtualDeviceTest.cpp | 17 +++++++++---- testing/VirtualDeviceTest.h | 30 +++++++++++++---------- testing/gtest.h | 36 ++++++++++++++++++++++++++++ testing/{fix-macros.h => iostream.h} | 16 ++++++------- testing/matchers.h | 11 ++++----- testing/setup-googletest.h | 17 +++++++------ tests/issues/951/test/testcase.cpp | 1 - 26 files changed, 165 insertions(+), 114 deletions(-) create mode 100644 testing/gtest.h rename testing/{fix-macros.h => iostream.h} (65%) diff --git a/testing/AbsoluteMouseReport.cpp b/testing/AbsoluteMouseReport.cpp index 7b9bc3da..33f9e485 100644 --- a/testing/AbsoluteMouseReport.cpp +++ b/testing/AbsoluteMouseReport.cpp @@ -16,12 +16,11 @@ #include "testing/AbsoluteMouseReport.h" -#include "Kaleidoscope.h" -#include "testing/fix-macros.h" +#include // for memcpy +#include // for vector -#include - -#include "MouseButtons.h" +#include "MouseButtons.h" // for MOUSE_LEFT, MOUSE_MIDDLE, MOUSE_NEXT, MOUSE_PREV, MOUS... +#include "kaleidoscope/Runtime.h" // for Runtime, Runtime_ namespace kaleidoscope { namespace testing { diff --git a/testing/AbsoluteMouseReport.h b/testing/AbsoluteMouseReport.h index 887c2c1b..cc204aa3 100644 --- a/testing/AbsoluteMouseReport.h +++ b/testing/AbsoluteMouseReport.h @@ -16,11 +16,11 @@ #pragma once -#include -#include +#include // for uint16_t, uint32_t, uint8_t, int8_t +#include // for vector -#include "DeviceAPIs/AbsoluteMouseAPI.h" -#include "HID-Settings.h" +#include "DeviceAPIs/AbsoluteMouseAPI.h" // for HID_MouseAbsoluteReport_Data_t +#include "HID-Settings.h" // for HID_REPORTID_MOUSE_ABSOLUTE namespace kaleidoscope { namespace testing { diff --git a/testing/ConsumerControlReport.cpp b/testing/ConsumerControlReport.cpp index 8239ae11..8f64f511 100644 --- a/testing/ConsumerControlReport.cpp +++ b/testing/ConsumerControlReport.cpp @@ -16,10 +16,10 @@ #include "testing/ConsumerControlReport.h" -#include "Kaleidoscope.h" -#include "testing/fix-macros.h" +#include // for memcpy +#include // for vector -#include +#include "kaleidoscope/Runtime.h" // for Runtime, Runtime_ namespace kaleidoscope { namespace testing { diff --git a/testing/ConsumerControlReport.h b/testing/ConsumerControlReport.h index 61857f72..93ca97d9 100644 --- a/testing/ConsumerControlReport.h +++ b/testing/ConsumerControlReport.h @@ -16,11 +16,11 @@ #pragma once -#include -#include +#include // for uint32_t, uint16_t, uint8_t +#include // for vector -#include "HID-Settings.h" -#include "MultiReport/ConsumerControl.h" +#include "HID-Settings.h" // for HID_REPORTID_CONSUMERCONTROL +#include "MultiReport/ConsumerControl.h" // for HID_ConsumerControlReport_Data_t namespace kaleidoscope { namespace testing { diff --git a/testing/ExpectedKeyboardReport.cpp b/testing/ExpectedKeyboardReport.cpp index bf1f014a..ba4e71b5 100644 --- a/testing/ExpectedKeyboardReport.cpp +++ b/testing/ExpectedKeyboardReport.cpp @@ -16,6 +16,9 @@ #include "testing/ExpectedKeyboardReport.h" +#include // for uint8_t, uint32_t +#include // for set + namespace kaleidoscope { namespace testing { diff --git a/testing/ExpectedKeyboardReport.h b/testing/ExpectedKeyboardReport.h index ce967a7b..789dd875 100644 --- a/testing/ExpectedKeyboardReport.h +++ b/testing/ExpectedKeyboardReport.h @@ -16,10 +16,10 @@ #pragma once -#include -#include -#include -#include +#include // for uint8_t, uint32_t +#include // for set + +#include "testing/iostream.h" // for string namespace kaleidoscope { namespace testing { diff --git a/testing/ExpectedMouseReport.cpp b/testing/ExpectedMouseReport.cpp index fb85d7e5..e6f938c8 100644 --- a/testing/ExpectedMouseReport.cpp +++ b/testing/ExpectedMouseReport.cpp @@ -16,6 +16,9 @@ #include "testing/ExpectedMouseReport.h" +#include "MultiReport/Mouse.h" // for (anonymous union)::(anonymous) +#include "testing/MouseReport.h" // for MouseReport::ReportData + namespace kaleidoscope { namespace testing { diff --git a/testing/ExpectedMouseReport.h b/testing/ExpectedMouseReport.h index 1d6f2e41..932de2c0 100644 --- a/testing/ExpectedMouseReport.h +++ b/testing/ExpectedMouseReport.h @@ -16,12 +16,10 @@ #pragma once -#include -#include -#include -#include +#include // for int8_t, uint32_t, uint8_t -#include "MouseReport.h" +#include "MouseReport.h" // for MouseReport, MouseReport::ReportData +#include "testing/iostream.h" // for string namespace kaleidoscope { namespace testing { diff --git a/testing/HIDState.cpp b/testing/HIDState.cpp index 68c3fb4c..4aac3448 100644 --- a/testing/HIDState.cpp +++ b/testing/HIDState.cpp @@ -16,12 +16,14 @@ #include "testing/HIDState.h" -#include "HID-Settings.h" +// IWYU pragma: no_include <__utility/move.h> -#include "testing/fix-macros.h" +#include // IWYU pragma: keep +#include // for vector + +#include "HID-Settings.h" // for HID_REPORTID_CONSUMERCONTROL, HID_REPORTID_GAMEPAD, HID_RE... +#include "testing/iostream.h" // for operator<<, char_traits, cout, ostream, basic_ostream -// TODO(epan): Add proper logging. -#include #define LOG(x) std::cout namespace kaleidoscope { diff --git a/testing/HIDState.h b/testing/HIDState.h index 9b93e770..af8449d9 100644 --- a/testing/HIDState.h +++ b/testing/HIDState.h @@ -16,15 +16,18 @@ #pragma once -#include "testing/AbsoluteMouseReport.h" -#include "testing/ConsumerControlReport.h" -#include "testing/KeyboardReport.h" -#include "testing/MouseReport.h" -#include "testing/SystemControlReport.h" - -// Out of order due to macro conflicts. -#include "testing/fix-macros.h" -#include +// IWYU pragma: no_include <__memory/unique_ptr.h> + +#include // for size_t +#include // for uint8_t +#include // IWYU pragma: keep +#include // for vector + +#include "testing/AbsoluteMouseReport.h" // for AbsoluteMouseReport +#include "testing/ConsumerControlReport.h" // for ConsumerControlReport +#include "testing/KeyboardReport.h" // for KeyboardReport +#include "testing/MouseReport.h" // for MouseReport +#include "testing/SystemControlReport.h" // for SystemControlReport namespace kaleidoscope { namespace testing { diff --git a/testing/KeyboardReport.cpp b/testing/KeyboardReport.cpp index 89bf822f..429e0f26 100644 --- a/testing/KeyboardReport.cpp +++ b/testing/KeyboardReport.cpp @@ -16,10 +16,11 @@ #include "testing/KeyboardReport.h" -#include "Kaleidoscope.h" -#include "testing/fix-macros.h" +#include // for memcpy +#include // for vector<>::iterator, vector -#include +#include "kaleidoscope/Runtime.h" // for Runtime, Runtime_ +#include "kaleidoscope/key_defs.h" // for HID_KEYBOARD_FIRST_MODIFIER, HID_LAST_KEY namespace kaleidoscope { namespace testing { diff --git a/testing/KeyboardReport.h b/testing/KeyboardReport.h index a798d28c..47d06e77 100644 --- a/testing/KeyboardReport.h +++ b/testing/KeyboardReport.h @@ -16,11 +16,11 @@ #pragma once -#include -#include +#include // for uint8_t, uint32_t +#include // for vector -#include "HID-Settings.h" -#include "MultiReport/Keyboard.h" +#include "HID-Settings.h" // for HID_REPORTID_NKRO_KEYBOARD +#include "MultiReport/Keyboard.h" // for HID_KeyboardReport_Data_t namespace kaleidoscope { namespace testing { diff --git a/testing/MouseReport.cpp b/testing/MouseReport.cpp index 49bb9121..a24b54e0 100644 --- a/testing/MouseReport.cpp +++ b/testing/MouseReport.cpp @@ -16,12 +16,9 @@ #include "testing/MouseReport.h" -#include "Kaleidoscope.h" -#include "testing/fix-macros.h" +#include // for memcpy -#include - -#include "MouseButtons.h" +#include "kaleidoscope/Runtime.h" // for Runtime, Runtime_ namespace kaleidoscope { namespace testing { diff --git a/testing/MouseReport.h b/testing/MouseReport.h index 8b4d0258..aa0f9333 100644 --- a/testing/MouseReport.h +++ b/testing/MouseReport.h @@ -16,11 +16,11 @@ #pragma once -#include -#include +#include // for uint8_t, int8_t, uint32_t -#include "HID-Settings.h" -#include "MultiReport/Mouse.h" +#include "HID-Settings.h" // for HID_REPORTID_MOUSE +#include "MouseButtons.h" // for MOUSE_LEFT, MOUSE_MIDDLE, MOUSE_NEXT, MOUSE_PREV, MOUSE_R... +#include "MultiReport/Mouse.h" // for HID_MouseReport_Data_t namespace kaleidoscope { namespace testing { diff --git a/testing/SimHarness.cpp b/testing/SimHarness.cpp index b6acb7e1..7f716d67 100644 --- a/testing/SimHarness.cpp +++ b/testing/SimHarness.cpp @@ -16,8 +16,10 @@ #include "testing/SimHarness.h" -#include "testing/fix-macros.h" -#include +#include // for millis + +#include "kaleidoscope/Runtime.h" // for Runtime, Runtime_ +#include "kaleidoscope/device/device.h" // for Device, VirtualProps::KeyScanner namespace kaleidoscope { namespace testing { diff --git a/testing/SimHarness.h b/testing/SimHarness.h index bda69547..424a43ca 100644 --- a/testing/SimHarness.h +++ b/testing/SimHarness.h @@ -16,11 +16,11 @@ #pragma once -#include -#include +#include // for size_t +#include // for uint8_t -#include "Kaleidoscope.h" -#include "testing/fix-macros.h" +#include "kaleidoscope/KeyAddr.h" // for KeyAddr +#include "testing/gtest.h" // IWYU pragma: keep namespace kaleidoscope { namespace testing { diff --git a/testing/State.h b/testing/State.h index b8951b5c..694fd5eb 100644 --- a/testing/State.h +++ b/testing/State.h @@ -16,15 +16,11 @@ #pragma once -#include -#include -#include +// IWYU pragma: no_include <__memory/unique_ptr.h> -#include "testing/HIDState.h" +#include // IWYU pragma: keep -// Out of order due to macro conflicts. -#include "testing/fix-macros.h" -#include +#include "testing/HIDState.h" // for HIDState namespace kaleidoscope { namespace testing { diff --git a/testing/SystemControlReport.cpp b/testing/SystemControlReport.cpp index 2fb7cc0a..206ee26c 100644 --- a/testing/SystemControlReport.cpp +++ b/testing/SystemControlReport.cpp @@ -16,10 +16,9 @@ #include "testing/SystemControlReport.h" -#include "Kaleidoscope.h" -#include "testing/fix-macros.h" +#include // for memcpy -#include +#include "kaleidoscope/Runtime.h" // for Runtime, Runtime_ namespace kaleidoscope { namespace testing { diff --git a/testing/SystemControlReport.h b/testing/SystemControlReport.h index c5d4edcf..741c565e 100644 --- a/testing/SystemControlReport.h +++ b/testing/SystemControlReport.h @@ -16,11 +16,11 @@ #pragma once -#include -#include +#include // for uint8_t, uint32_t +#include // for vector -#include "HID-Settings.h" -#include "MultiReport/SystemControl.h" +#include "HID-Settings.h" // for HID_REPORTID_SYSTEMCONTROL +#include "MultiReport/SystemControl.h" // for HID_SystemControlReport_Data_t namespace kaleidoscope { namespace testing { diff --git a/testing/VirtualDeviceTest.cpp b/testing/VirtualDeviceTest.cpp index e7da0619..0bf3282e 100644 --- a/testing/VirtualDeviceTest.cpp +++ b/testing/VirtualDeviceTest.cpp @@ -16,10 +16,19 @@ #include "testing/VirtualDeviceTest.h" -#include "HIDReportObserver.h" -#include "testing/HIDState.h" - -#include +// IWYU pragma: no_include <__algorithm/max.h> +// IWYU pragma: no_include <__tree> + +#include // for bitset +#include // for vector + +#include "HIDReportObserver.h" // for HIDReportObserver +#include "kaleidoscope/Runtime.h" // for Runtime, Runtime_ +#include "testing/HIDState.h" // for HIDState, HIDStateBuilder +#include "testing/KeyboardReport.h" // for KeyboardReport +#include "testing/MouseReport.h" // for MouseReport +#include "testing/gtest.h" // for Message, TestPartResult, EXPECT_EQ, ElementsAreArray +#include "testing/iostream.h" // for operator<<, basic_ostream, char_traits, string, cerr namespace kaleidoscope { namespace testing { diff --git a/testing/VirtualDeviceTest.h b/testing/VirtualDeviceTest.h index 9231c4cd..2c40fdc0 100644 --- a/testing/VirtualDeviceTest.h +++ b/testing/VirtualDeviceTest.h @@ -16,18 +16,24 @@ #pragma once -#include - -#include "testing/ExpectedKeyboardReport.h" -#include "testing/ExpectedMouseReport.h" -#include "testing/SimHarness.h" -#include "testing/State.h" - -// Out of order due to macro conflicts. -#include "testing/fix-macros.h" -#include "gmock/gmock.h" -#include "gtest/gtest.h" -#include +// IWYU pragma: no_include <__memory/unique_ptr.h> + +#include // for size_t +#include // for uint32_t, int8_t, uint8_t +#include // for initializer_list +#include // IWYU pragma: keep +#include // for set +#include // for vector + +#include "kaleidoscope/KeyAddr.h" // for KeyAddr +#include "kaleidoscope/key_defs.h" // for Key +#include "testing/ExpectedKeyboardReport.h" // for ExpectedKeyboardReport +#include "testing/ExpectedMouseReport.h" // for ExpectedMouseReport +#include "testing/HIDState.h" // for HIDState +#include "testing/SimHarness.h" // for SimHarness +#include "testing/State.h" // for State +#include "testing/gtest.h" // for Test +#include "testing/iostream.h" // for string namespace kaleidoscope { namespace testing { diff --git a/testing/gtest.h b/testing/gtest.h new file mode 100644 index 00000000..1931e629 --- /dev/null +++ b/testing/gtest.h @@ -0,0 +1,36 @@ +/* -*- mode: c++ -*- + * Copyright (C) 2022 Keyboardio, Inc. + * + * This program is free software: you can redistribute it and/or modify it under + * the terms of the GNU General Public License as published by the Free Software + * Foundation, version 3. + * + * This program is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS + * FOR A PARTICULAR PURPOSE. See the GNU General Public License for more + * details. + * + * You should have received a copy of the GNU General Public License along with + * this program. If not, see . + */ + +#pragma once + +#undef TEST + +// In order to include gtest files, we need to undefine some macros that +// Arduino.h unwisely exports. Rahter than including "gtest/gtest.h" (et al) +// directly, any simulator code should instead include "testing/gtest.h". +#undef min +#undef max + +// The headers listed here other than "gtest/gtest.h" and "gmock/gmock.h" are +// only included to prevent IWYU from inserting them directly into simulator +// source files. This seems mildly preferable to modifying the gtest files +// themselves. +#include "gmock/gmock-matchers.h" // IWYU pragma: export +#include "gmock/gmock.h" // IWYU pragma: export +#include "gtest/gtest-message.h" // IWYU pragma: export +#include "gtest/gtest-test-part.h" // IWYU pragma: export +#include "gtest/gtest.h" // IWYU pragma: export +#include "gtest/gtest_pred_impl.h" // IWYU pragma: export diff --git a/testing/fix-macros.h b/testing/iostream.h similarity index 65% rename from testing/fix-macros.h rename to testing/iostream.h index a6a63c81..40dca8f7 100644 --- a/testing/fix-macros.h +++ b/testing/iostream.h @@ -1,5 +1,5 @@ /* -*- mode: c++ -*- - * Copyright (C) 2020 Eric Paniagua (epaniagua@google.com) + * Copyright (C) 2022 Keyboardio, Inc. * * This program is free software: you can redistribute it and/or modify it under * the terms of the GNU General Public License as published by the Free Software @@ -14,12 +14,12 @@ * this program. If not, see . */ -// No `#pragma once` since these undefs need to have every time a Kaleidoscope/ -// Arduino header is included before non-Kaleidoscope/Arduino header. The undefs -// are needed, due to naming conflicts between Arduino and Googletest. +#pragma once -#undef min +// In order to include certain standard library files, we need to undefine some +// macros that Arduino.h unwisely exports. Rahter than including +// directly, any simulator code should instead include "testing/iostream.h". #undef max -#undef T -#undef U -#undef TEST +#undef min + +#include // IWYU pragma: export diff --git a/testing/matchers.h b/testing/matchers.h index bd717568..6de9fe0e 100644 --- a/testing/matchers.h +++ b/testing/matchers.h @@ -16,13 +16,12 @@ #pragma once -#include "kaleidoscope/key_defs.h" -#include "testing/SystemControlReport.h" +// For some reason, the `MATCHER_P` macro confuses IWYU into trying to include +// this file in itself, so we need to block it with the following: +// IWYU pragma: no_include "testing/matchers.h" -// Out of order because `fix-macros.h` clears the preprocessor environment for -// gtest and gmock. -#include "testing/fix-macros.h" -#include "gmock/gmock.h" +#include "kaleidoscope/key_defs.h" // for Key +#include "testing/gtest.h" // for GMOCK_PP_INTERNAL_FOR_EACH_IMPL_1, GMOCK_PP_INTERNAL_... namespace kaleidoscope { namespace testing { diff --git a/testing/setup-googletest.h b/testing/setup-googletest.h index 85cb034e..b7830d89 100644 --- a/testing/setup-googletest.h +++ b/testing/setup-googletest.h @@ -18,17 +18,16 @@ #pragma once -#include "kaleidoscope/key_defs/keyboard.h" -#include "Kaleidoscope.h" +#include // IWYU pragma: keep -// Out of order because `fix-macros.h` clears the preprocessor environment for -// gtest and gmock. -#include "testing/fix-macros.h" -#include "gmock/gmock.h" -#include "gtest/gtest.h" +// Kaleidoscope.h includes Arduino, which unwisely defines `min` and `max` as +// preprocessor macros. We need to undefine these macros before including any +// files from the standard library. +#undef min +#undef max -#include "testing/matchers.h" -#include "testing/VirtualDeviceTest.h" +#include "testing/VirtualDeviceTest.h" // IWYU pragma: keep +#include "testing/matchers.h" // IWYU pragma: keep #define SETUP_GOOGLETEST() \ void executeTestFunction() { \ diff --git a/tests/issues/951/test/testcase.cpp b/tests/issues/951/test/testcase.cpp index ffe19b84..e69cb1b3 100644 --- a/tests/issues/951/test/testcase.cpp +++ b/tests/issues/951/test/testcase.cpp @@ -15,7 +15,6 @@ */ #include "Kaleidoscope.h" -#include "testing/fix-macros.h" #include "testing/setup-googletest.h" From 7db4fca6c09e031fe3e66a2261569abde71fd735 Mon Sep 17 00:00:00 2001 From: Michael Richters Date: Mon, 11 Apr 2022 19:25:32 -0500 Subject: [PATCH 09/10] Allow override of clang-format executable Because GitHub's Ubuntu image offers multiple version fo clang-format, and sometimes it seems to prefer a version lower than 12 (which is the minimum required by our config file), I added an override via the new environment variable `KALEIDOSCOPE_CODE_FORMATTER`. Signed-off-by: Michael Richters --- .github/workflows/build.yml | 2 +- bin/format-code.py | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 2a2d0f93..0cdc31b2 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -36,7 +36,7 @@ jobs: runs-on: ubuntu-latest steps: - uses: actions/checkout@v2 - - run: make check-code-style + - run: KALEIDOSCOPE_CODE_FORMATTER=clang-format-12 make check-code-style check-shellcheck: runs-on: ubuntu-latest steps: diff --git a/bin/format-code.py b/bin/format-code.py index 59a464d4..f8eaf963 100755 --- a/bin/format-code.py +++ b/bin/format-code.py @@ -172,7 +172,9 @@ def main(): return 1 # Locate `clang-format` executable: - clang_format_exe = shutil.which('clang-format') + clang_format_exe = os.getenv('KALEIDOSCOPE_CODE_FORMATTER') + if clang_format_exe is None: + clang_format_exe = shutil.which('clang-format') logging.debug("Found `clang-format` executable: %s", clang_format_exe) clang_format_cmd = [clang_format_exe, '-i'] if opts.loglevel <= logging.INFO: From f8237165c194705829e6ea7d25edb7c3724a054a Mon Sep 17 00:00:00 2001 From: Michael Richters Date: Tue, 12 Apr 2022 11:33:07 -0500 Subject: [PATCH 10/10] Add documentation for `format-code.py` and `iwyu.py` I put this in the code style guide, for lack of a better place. Signed-off-by: Michael Richters --- docs/codebase/code-style.md | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/docs/codebase/code-style.md b/docs/codebase/code-style.md index c5fea013..efc9635d 100644 --- a/docs/codebase/code-style.md +++ b/docs/codebase/code-style.md @@ -132,6 +132,7 @@ Our style guide is based on the [Google C++ style guide][goog:c++-guide] which w - [Vertical Whitespace](#vertical-whitespace) - [Exceptions to the Rules](#exceptions-to-the-rules) - [Existing Non-conformant Code](#existing-non-conformant-code) + - [Maintenance Tools](#maintenance-tools) - [Parting Words](#parting-words) ## Background @@ -3155,6 +3156,40 @@ The coding conventions described above are mandatory. However, like all good rul If you find yourself modifying code that was written to specifications other than those presented by this guide, you may have to diverge from these rules in order to stay consistent with the local conventions in that code. If you are in doubt about how to do this, ask the original author or the person currently responsible for the code. Remember that *consistency* includes local consistency, too. +## Maintenance Tools + +Kaleidoscope uses some automated tools to enforce compliance with this code style guide. Primarily, we use `clang-format` to format source files, `cpplint` to check for potential problems, and `include-what-you-use` to update header includes. These are invoked using python scripts that supply all of the necessary command-line parameters to both utilities. For convenience, there are also some shell scripts and makefile targets that further simplify the process of running these utilities properly. + +### Code Formatting + +We use `clang-format`(version 12 or higher) to automatically format Kaleidoscope source files. There is a top-level `.clang-format` config file that contains the settings that best match the style described in this guide. However, there are some files in the repository that are, for one reason or another, exempt from formatting in this way, so we use a wrapper script, `format-code.py`, instead of invoking it directly. + +`format-code.py` takes a list of target filenames, either as command-line parameters or from standard input (if reading from a pipe, with each line treated as a filename), and formats those files. If given a directory target, it will recursively search that directory for source files, and format all of the ones it finds. + +By default, `format-code.py` will first check for unstaged changes in the Kaleidoscope git working tree, and exit before formatting source files if any are found. This is meant to make it easier for developers to see the changes made by the formatter in isolation. It can be given the `--force` option to skip this check. + +It also has a `--check` option, which will cause `format-code.py` to check for unstaged git working tree changes after running `clang-format` on the target source files, and return an error code if there are any, allowing us to automatically verify that code submitted complies with the formatting rules. + +The easiest way to invoke the formatter on the whole repository is by running `make format` at the top level of the Kaleidoscope repository. + +For automated checking of PRs in a CI tool, there is also `make check-code-style`. + +### Linting + +We use a copy of `cpplint.py` (with a modification that allows us to set the config file) to check for potential problems in the code. This can be invoked by running `make cpplint` at the top level of the Kaleidoscope repository. + +### Header Includes + +We use `include-what-you-use` (version 18 or higher) to automatically manage header includes in Kaleidoscope source files. Because of the peculiarities of Kaleidoscope's build system, we use a wrapper script, `iwyu.py`, instead of invoking it directly. + +`iwyu.py` takes a list of target filenames, either as command-line parameters or from standard input (if reading from a pipe, with each line treated as a filename), and makes changes to the header includes. If given a directory target, it will recursively search that directory for source files, and run `include-what-you-use` on all of the ones it finds. + +A number of files can't be processed this way, and are enumerated in `.iwyu_ignore`. Files in the `testing` directory (for the test simulator) require different include path items, so cannot be combined in the same call to `iwyu.py`. + +The easiest way to invoke `iwyu.py` is by running `make check-includes`, which will check all files that differ between the git working tree and the current master branch of the main Kaleidoscope repository, update their headers, and return a non-zero exit code if there were any errors processing the file(s) or any changes made. + +For automated checking of header includes in a CI tool, there is also `make check-all-includes`, that checks the whole repository, not just the current branch's changes. + ## Parting Words Use common sense and *BE CONSISTENT*.