[libcamera-devel] [PATCH v2 2/2] utils: checkstyle: Add support for clang-format
Niklas Söderlund
niklas.soderlund at ragnatech.se
Tue Jan 22 12:25:16 CET 2019
Hi Laurent,
Thanks for your work.
On 2019-01-21 20:01:28 +0200, Laurent Pinchart wrote:
> clang-format produces better results than astyle as it can better match
> the libcamera coding style. Default to clang-format over astyle, fall
> back to astyle if clang-format isn't found, and add a --formatter
> command line option to select a formatter manually.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
With the comment Kieran points out,
Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
> Documentation/coding-style.rst | 25 ++++++++----
> utils/checkstyle.py | 70 +++++++++++++++++++++++++---------
> 2 files changed, 71 insertions(+), 24 deletions(-)
>
> diff --git a/Documentation/coding-style.rst b/Documentation/coding-style.rst
> index 30a1455d8c65..02cdc1677bf1 100644
> --- a/Documentation/coding-style.rst
> +++ b/Documentation/coding-style.rst
> @@ -170,8 +170,11 @@ These rules match the `object ownership rules from the Chromium C++ Style Guide`
> Tools
> -----
>
> -The 'astyle' code formatting tool can be used to reformat source files with the
> -libcamera coding style, defined by the following arguments.
> +The 'clang-format' code formatting tool can be used to reformat source files
> +with the libcamera coding style, defined in the .clang-format file at the root
> +of the source tree.
> +
> +Alternatively the 'astyle' tool can also be used, with the following arguments.
>
> ::
>
> @@ -184,11 +187,15 @@ libcamera coding style, defined by the following arguments.
> --align-reference=name
> --max-code-length=120
>
> -As astyle is a code formatter, it operates on full files outputs reformatted
> -source code. While it can be used to reformat code before sending patches, it
> -may generate unrelated changes. To avoid this, libcamera provides a
> -'checkstyle.py' script wrapping astyle to only retain related changes. This
> -should be used to validate modifications before submitting them for review.
> +Use of astyle is discourage as clang-format better matches the libcamera coding
> +style.
> +
> +As both astyle and clang-format are code formatters, they operate on full files
> +and output reformatted source code. While they can be used to reformat code
> +before sending patches, it may generate unrelated changes. To avoid this,
> +libcamera provides a 'checkstyle.py' script wrapping the formatting tools to
> +only retain related changes. This should be used to validate modifications
> +before submitting them for review.
>
> The script operates on one or multiple git commits specified on the command
> line. It does not modify the git tree, the index or the working directory and
> @@ -277,4 +284,8 @@ diff that fixes the issues, on top of the corresponding commit. As the script is
> in early development false positive are expected. The flagged issues should be
> reviewed, but the diff doesn't need to be applied blindly.
>
> +The checkstyle.py script uses clang-format by default if found, and otherwise
> +falls back to astyle. The formatter can be manually selected with the
> +'--formatter' argument.
> +
> Happy hacking, libcamera awaits your patches!
> diff --git a/utils/checkstyle.py b/utils/checkstyle.py
> index a40d7dec879f..f3005d1fbb52 100755
> --- a/utils/checkstyle.py
> +++ b/utils/checkstyle.py
> @@ -4,11 +4,11 @@
> #
> # Author: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> #
> -# checkstyle.py - A patch style checker script based on astyle
> +# checkstyle.py - A patch style checker script based on astyle or clang-format
> #
> # TODO:
> #
> -# - Support other formatting tools (clang-format, ...)
> +# - Support other formatting tools and checkers (cppcheck, cpplint, kwstyle, ...)
> # - Split large hunks to minimize context noise
> # - Improve style issues counting
> #
> @@ -33,6 +33,12 @@ astyle_options = (
> '--max-code-length=120'
> )
>
> +dependencies = {
> + 'astyle': False,
> + 'clang-format': False,
> + 'git': True,
> +}
> +
> source_extensions = (
> '.c',
> '.cpp',
> @@ -192,30 +198,38 @@ def parse_diff(diff):
> # Code reformatting
> #
>
> -def formatter_astyle(data):
> +def formatter_astyle(filename, data):
> ret = subprocess.run(['astyle', *astyle_options],
> input=data.encode('utf-8'), stdout=subprocess.PIPE)
> return ret.stdout.decode('utf-8')
>
>
> -def formatter_strip_trailing_space(data):
> +def formatter_clang_format(filename, data):
> + ret = subprocess.run(['clang-format', '-style=file',
> + '-assume-filename=' + filename],
> + input=data.encode('utf-8'), stdout=subprocess.PIPE)
> + return ret.stdout.decode('utf-8')
> +
> +
> +def formatter_strip_trailing_space(filename, data):
> lines = data.split('\n')
> for i in range(len(lines)):
> lines[i] = lines[i].rstrip() + '\n'
> return ''.join(lines)
>
>
> -formatters = [
> - formatter_astyle,
> - formatter_strip_trailing_space,
> -]
> +available_formatters = {
> + 'astyle': formatter_astyle,
> + 'clang-format': formatter_clang_format,
> + 'strip-trailing-spaces': formatter_strip_trailing_space,
> +}
>
>
> # ------------------------------------------------------------------------------
> # Style checking
> #
>
> -def check_file(top_level, commit, filename):
> +def check_file(top_level, commit, filename, formatters):
> # Extract the line numbers touched by the commit.
> diff = subprocess.run(['git', 'diff', '%s~..%s' % (commit, commit), '--',
> '%s/%s' % (top_level, filename)],
> @@ -239,7 +253,8 @@ def check_file(top_level, commit, filename):
>
> formatted = after
> for formatter in formatters:
> - formatted = formatter(formatted)
> + formatter = available_formatters[formatter]
> + formatted = formatter(filename, formatted)
>
> after = after.splitlines(True)
> formatted = formatted.splitlines(True)
> @@ -261,7 +276,7 @@ def check_file(top_level, commit, filename):
> return len(formatted_diff)
>
>
> -def check_style(top_level, commit):
> +def check_style(top_level, commit, formatters):
> # Get the commit title and list of files.
> ret = subprocess.run(['git', 'show', '--pretty=oneline','--name-only', commit],
> stdout=subprocess.PIPE)
> @@ -282,7 +297,7 @@ def check_style(top_level, commit):
>
> issues = 0
> for f in files:
> - issues += check_file(top_level, commit, f)
> + issues += check_file(top_level, commit, f, formatters)
>
> if issues == 0:
> print("No style issue detected")
> @@ -330,17 +345,38 @@ def main(argv):
>
> # Parse command line arguments
> parser = argparse.ArgumentParser()
> + parser.add_argument('--formatter', '-f', type=str, choices=['astyle', 'clang-format'],
> + help='Code formatter. Default to clang-format if not specified.')
> parser.add_argument('revision_range', type=str, default='HEAD', nargs='?',
> help='Revision range (as defined by git rev-parse). Defaults to HEAD if not specified.')
> args = parser.parse_args(argv[1:])
>
> # Check for required dependencies.
> - dependencies = ('astyle', 'git')
> + for command, mandatory in dependencies.items():
> + found = shutil.which(command)
> + if mandatory and not found:
> + print("Executable %s not found" % command)
> + return 1
>
> - for dependency in dependencies:
> - if not shutil.which(dependency):
> - print("Executable %s not found" % dependency)
> + dependencies[command] = found
> +
> + if args.formatter:
> + if not args.formatter in dependencies or \
> + not dependencies[args.formatter]:
> + print("Formatter %s not available" % args.formatter)
> return 1
> + formatter = args.formatter
> + else:
> + if dependencies['clang-format']:
> + formatter = 'clang-format'
> + elif dependencies['astyle']:
> + formatter = 'astyle'
> + else:
> + print("No formatter found, please install clang-format or astyle")
> + return 1
> +
> + # Create the list of formatters to be applied.
> + formatters = [formatter, 'strip-trailing-spaces']
>
> # Get the top level directory to pass absolute file names to git diff
> # commands, in order to support execution from subdirectories of the git
> @@ -352,7 +388,7 @@ def main(argv):
> revlist = extract_revlist(args.revision_range)
>
> for commit in revlist:
> - check_style(top_level, commit)
> + check_style(top_level, commit, formatters)
> print('')
>
> return 0
> --
> Regards,
>
> Laurent Pinchart
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
--
Regards,
Niklas Söderlund
More information about the libcamera-devel
mailing list