[libcamera-devel,v2,2/2] utils: checkstyle: Add support for clang-format

310 diff mbox series

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@ideasonboard.com>
---
 Documentation/coding-style.rst | 25 ++++++++----
 utils/checkstyle.py            | 70 +++++++++++++++++++++++++---------
 2 files changed, 71 insertions(+), 24 deletions(-)

Hi Laurent,

On 21/01/2019 18:01, 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.


All I can find is a minor spelling error below:

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  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

s/discourage/discouraged/

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

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@ideasonboard.com>

With the comment Kieran points out,

Reviewed-by: Niklas Söderlund <niklas.soderlund@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@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@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel