[libcamera-devel] [PATCH] clang-format: Support multiple versions

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Fri, Aug 27, 2021 at 03:47:53PM +0100, Kieran Bingham wrote:
> The .clang-format file can use CaseSensitive on Regex rules for Include
> Categories. With this we can distinguish between QT headers which

s/QT/Qt/

> commence with a 'Q' verses system headers including <queue>.
> 
> This requires clang-format-12 to support that rule, and clang-format is
> not very amicable to .clang-format files with unsupported entries, and
> will fail to run on previous versions if we add that option directly.
> 
> This causes the checkstyle utility to become unusable for users of
> clang-format versions prior to 12.
> 
> Unfortunately, clang-format does not provide a way to specify a specific
> file to use for the rules to apply when formatting, so we can not easily
> call clang-format with a file specific to its versioned needs.
> 
> Implement a means of identifying the version of clang-format, and use
> that information to search for the most recent supportable
> .clang-format-$(version) file, which can then be symlinked to the fixed
> file location used by clang format (.clang-format).
> 
>  .clang-format-12: is added with CaseSensitive:true for QT headers

Ditto.

>  .clang-format-7: is moved from the current .clang-format implementation
> 
> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> 
> ---
> Note there are few things I dislike about this, hence RFC:
> 
>  - We leave our tree without a .clang-format until checkstyle runs..

A bit annoying, but I think we can live with that. Maybe it's a good way
to encourage usage of checkstyle.py :-)

>  - We run the unlinking and symlinking on every invocation of checkstyle

Shouldn't be too costly :-) It's slightly annoying that we can't run
checkstyle.py on a read-only directory anymore though, but I don't think
it's a common use case.

>  - The 'autogenerated' file in place of a git tracked one causes
>    issues on rebase. Could be an issue for bisects...?

Possibly, but I don't think we'll often bisect and run checkstyle.py at
the same time.

> 
>  .clang-format-12                 | 169 +++++++++++++++++++++++++++++++
>  .clang-format => .clang-format-7 |   0
>  utils/checkstyle.py              |  49 ++++++++-

The symlink should be added to .gitignore.

>  3 files changed, 217 insertions(+), 1 deletion(-)
>  create mode 100644 .clang-format-12
>  rename .clang-format => .clang-format-7 (100%)
> 
> diff --git a/.clang-format-12 b/.clang-format-12
> new file mode 100644
> index 000000000000..98e9fad472fc
> --- /dev/null
> +++ b/.clang-format-12
> @@ -0,0 +1,169 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +#
> +# clang-format configuration file. Intended for clang-format >= 12.
> +#
> +# For more information, see:
> +#
> +#   Documentation/process/clang-format.rst
> +#   https://clang.llvm.org/docs/ClangFormat.html
> +#   https://clang.llvm.org/docs/ClangFormatStyleOptions.html
> +#
> +---
> +Language: Cpp
> +AccessModifierOffset: -8
> +AlignAfterOpenBracket: Align
> +AlignConsecutiveAssignments: false
> +AlignConsecutiveDeclarations: false
> +AlignEscapedNewlines: Right
> +AlignOperands: true
> +AlignTrailingComments: false
> +AllowAllParametersOfDeclarationOnNextLine: false
> +AllowShortBlocksOnASingleLine: false
> +AllowShortCaseLabelsOnASingleLine: false
> +AllowShortFunctionsOnASingleLine: InlineOnly
> +AllowShortIfStatementsOnASingleLine: false
> +AllowShortLoopsOnASingleLine: false
> +AlwaysBreakAfterDefinitionReturnType: None
> +AlwaysBreakAfterReturnType: None
> +AlwaysBreakBeforeMultilineStrings: false
> +AlwaysBreakTemplateDeclarations: MultiLine
> +BinPackArguments: true
> +BinPackParameters: true
> +BraceWrapping:
> +  AfterClass: true
> +  AfterControlStatement: false
> +  AfterEnum: false
> +  AfterFunction: true
> +  AfterNamespace: false
> +  AfterObjCDeclaration: false
> +  AfterStruct: false
> +  AfterUnion: false
> +  AfterExternBlock: false
> +  BeforeCatch: false
> +  BeforeElse: false
> +  IndentBraces: false
> +  SplitEmptyFunction: true
> +  SplitEmptyRecord: true
> +  SplitEmptyNamespace: true
> +BreakBeforeBinaryOperators: None
> +BreakBeforeBraces: Custom
> +BreakBeforeInheritanceComma: false
> +BreakInheritanceList: BeforeColon
> +BreakBeforeTernaryOperators: true
> +BreakConstructorInitializers: BeforeColon
> +BreakAfterJavaFieldAnnotations: false
> +BreakStringLiterals: false
> +ColumnLimit: 0
> +CommentPragmas: '^ IWYU pragma:'
> +CompactNamespaces: false
> +ConstructorInitializerAllOnOneLineOrOnePerLine: false
> +ConstructorInitializerIndentWidth: 8
> +ContinuationIndentWidth: 8
> +Cpp11BracedListStyle: false
> +DerivePointerAlignment: false
> +DisableFormat: false
> +ExperimentalAutoDetectBinPacking: false
> +FixNamespaceComments: true
> +ForEachMacros:
> +  - 'udev_list_entry_foreach'
> +SortIncludes: true
> +IncludeBlocks: Regroup
> +IncludeCategories:
> +  # Headers matching the name of the component are matched automatically.
> +  # Priority 1
> +  # Other library headers (explicit overrides to match before system headers)
> +  - Regex:           '(<jpeglib.h>|<libudev.h>|<tiffio.h>|<xf86drm.h>|<xf86drmMode.h>|<yaml.h>)'
> +    Priority:        9
> +  # Qt includes (match before C++ standard library)
> +  - Regex:           '<Q([A-Za-z0-9\-_])+>'
> +    Priority:        9
> +    CaseSensitive:   true
> +  # Headers in <> with an extension. (+system libraries)
> +  - Regex:           '<([A-Za-z0-9\-_])+\.h>'
> +    Priority:        2
> +  # System headers
> +  - Regex:           '<sys/.*>'
> +    Priority:        2
> +  # C++ standard library includes (no extension)
> +  - Regex:           '<([A-Za-z0-9\-_/])+>'
> +    Priority:        2
> +  # Linux headers, as a second group/subset of system headers
> +  - Regex:           '<linux/.*>'
> +    Priority:        3
> +  # Headers for libcamera Base support
> +  - Regex:           '<libcamera/base/private.h>'
> +    Priority:        4
> +  - Regex:           '<libcamera/base/.*\.h>'
> +    Priority:        5
> +  # Public API Headers for libcamera, which are not in a subdir (i.e. ipa/,internal/)
> +  - Regex:           '<libcamera/([A-Za-z0-9\-_])+.h>'
> +    Priority:        6
> +  # IPA Interfaces
> +  - Regex:           '<libcamera/ipa/.*\.h>'
> +    Priority:        7
> +  # libcamera Internal headers in ""
> +  - Regex:           '"libcamera/internal/.*\.h"'
> +    Priority:        8
> +  # Other libraries headers with one group per library (.h or .hpp)
> +  - Regex:           '<.*/.*\.hp*>'
> +    Priority:        9
> +  # local modular includes "path/file.h" (.h or .hpp)
> +  - Regex:           '"(.*/)+.*\.hp*"'
> +    Priority:        10
> +  # Other local headers "file.h" with extension (.h or .hpp)
> +  - Regex:           '".*.hp*"'
> +    Priority:        11
> +  # Any unmatched line, separated from the last group
> +  - Regex:	     '"*"'
> +    Priority:        100
> +
> +IncludeIsMainRegex: '(_test)?$'
> +IndentCaseLabels: false
> +IndentPPDirectives: None
> +IndentWidth: 8
> +IndentWrappedFunctionNames: false
> +JavaScriptQuotes: Leave
> +JavaScriptWrapImports: true
> +KeepEmptyLinesAtTheStartOfBlocks: false
> +MacroBlockBegin: ''
> +MacroBlockEnd: ''
> +MaxEmptyLinesToKeep: 1
> +NamespaceIndentation: None
> +ObjCBinPackProtocolList: Auto
> +ObjCBlockIndentWidth: 8
> +ObjCSpaceAfterProperty: true
> +ObjCSpaceBeforeProtocolList: true
> +
> +# Taken from git's rules
> +PenaltyBreakAssignment: 10
> +PenaltyBreakBeforeFirstCallParameter: 30
> +PenaltyBreakComment: 10
> +PenaltyBreakFirstLessLess: 0
> +PenaltyBreakString: 10
> +PenaltyBreakTemplateDeclaration: 10
> +PenaltyExcessCharacter: 100
> +PenaltyReturnTypeOnItsOwnLine: 60
> +
> +PointerAlignment: Right
> +ReflowComments: false
> +SortIncludes: true
> +SortUsingDeclarations: true
> +SpaceAfterCStyleCast: false
> +SpaceAfterTemplateKeyword: false
> +SpaceBeforeAssignmentOperators: true
> +SpaceBeforeCpp11BracedList: false
> +SpaceBeforeCtorInitializerColon: true
> +SpaceBeforeInheritanceColon: true
> +SpaceBeforeParens: ControlStatements
> +SpaceBeforeRangeBasedForLoopColon: true
> +SpaceInEmptyParentheses: false
> +SpacesBeforeTrailingComments: 1
> +SpacesInAngles: false
> +SpacesInContainerLiterals: false
> +SpacesInCStyleCastParentheses: false
> +SpacesInParentheses: false
> +SpacesInSquareBrackets: false
> +Standard: Cpp11
> +TabWidth: 8
> +UseTab: Always
> +...

I trust you on this.

> diff --git a/.clang-format b/.clang-format-7
> similarity index 100%
> rename from .clang-format
> rename to .clang-format-7
> diff --git a/utils/checkstyle.py b/utils/checkstyle.py
> index ececb46eaacc..279ace8346b4 100755
> --- a/utils/checkstyle.py
> +++ b/utils/checkstyle.py
> @@ -16,6 +16,7 @@
>  import argparse
>  import difflib
>  import fnmatch
> +import glob
>  import os.path
>  import re
>  import shutil
> @@ -566,9 +567,55 @@ class Formatter(metaclass=ClassRegistry):
>          return patterns
>  
>  
> -class CLangFormatter(Formatter):
> +# Identify the version of clang-format running, and match it against locally
> +# available versioned configuration files of the form:
> +# .clang-format-$(version)
> +#
> +# The highest supported local configuration file is linked as .clang-format by
> +# ClangFormatLinker.relink() for use by the ClangFormatter.
> +#
> +class ClangFormatLinker():

No need for parentheses if you don't inherit from another class, but you
can inherit from object.

Do we need ClangFormatLinker though ? Couldn't those functions be move
to ClangFormatter ? Although relinking should probably be done from
main(), see below.

> +    def version():
> +        version_regex = re.compile('[^0-9]*([0-9]+).*')
> +        ret = subprocess.run(['clang-format', '--version'], stdout=subprocess.PIPE)

What happens if clang-format isn't available ? We check for it in
main(), but I think this code will run before that.

> +        version = ret.stdout.decode('utf-8')
> +        search = version_regex.search(version)
> +        if search:
> +            version = search.group(1)
> +            return int(version)
> +
> +        # Lowest supported version
> +        return 7

Shouldn't this cause an error instead ?

> +
> +    def supported():
> +        version = ClangFormatLinker.version()

That's a bit of an abuse of the Python class system. Instance methods
are supposed to be called on an instance and take a self argument. These
three functions should be declared as class method I think. This would
become

    @classmethod
    def supported(cls):
        version = cls.version()

> +
> +        # Match the highest supported version
> +        clang_files = glob.glob('.clang-format-[0-9]*')

When checkstyle.py is run manually, it may be run from a subdirectory of
the git tree. You should pass the git top-level directory to relink().
This would then call for calling relink() directly from main() instead
of ClangFormatter.

> +        clang_files = sorted(clang_files, reverse=True,
> +                             key=lambda s: [int(re.sub("[^0-9]", "", s))])

We use single quotes for strings when possible.

> +        for f in clang_files:
> +            file_version = int(re.sub("[^0-9]", "", f))
> +            if file_version <= version:
> +                return f
> +
> +        return None
> +
> +    def relink():
> +        supported = ClangFormatLinker.supported()
> +        if not supported:
> +            return
> +
> +        if os.path.exists('.clang-format'):
> +            os.unlink('.clang-format')
> +        os.symlink(supported, '.clang-format')

Could we avoid relinking if the symlink points to the right file already
?

> +
> +
> +class ClangFormatter(Formatter):

CLangFormatter became ClangFormatter. Is it intentional ?

>      patterns = ('*.c', '*.cpp', '*.h')
>  
> +    ClangFormatLinker.relink()
> +
>      @classmethod
>      def format(cls, filename, data):
>          ret = subprocess.run(['clang-format', '-style=file',

-- 
Regards,

Laurent Pinchart