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

[Kieran Bingham]

On 27/08/2021 16:43, Laurent Pinchart wrote:
> 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.

Probably not, I was just trying to group this code together really, and
wanted to break out into functions a little to make it clearer.


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

Probably - we expect there to be a clang-format, and if we can't
interrogate it - then it's an issue.


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

Ok

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

Probably, - this was just the quick and dirty RFC (where I forgot to add
--rfc to git-format-patch :D)

Presumably os.readlink() will give us something we can work with ...

>> +
>> +
>> +class ClangFormatter(Formatter):
> 
> CLangFormatter became ClangFormatter. Is it intentional ?

Yes, but it was supposed to be in a separate patch.

I don't think I've ever seen anyone really call it "C Lang" rather than
"Clang"



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