[{"id":19151,"web_url":"https://patchwork.libcamera.org/comment/19151/","msgid":"","date":"2021-08-27T15:43:25","subject":"Re: [libcamera-devel] [PATCH] clang-format: Support multiple\n\tversions","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nThank you for the patch.\n\nOn Fri, Aug 27, 2021 at 03:47:53PM +0100, Kieran Bingham wrote:\n> The .clang-format file can use CaseSensitive on Regex rules for Include\n> Categories. With this we can distinguish between QT headers which\n\ns/QT/Qt/\n\n> commence with a 'Q' verses system headers including .\n> \n> This requires clang-format-12 to support that rule, and clang-format is\n> not very amicable to .clang-format files with unsupported entries, and\n> will fail to run on previous versions if we add that option directly.\n> \n> This causes the checkstyle utility to become unusable for users of\n> clang-format versions prior to 12.\n> \n> Unfortunately, clang-format does not provide a way to specify a specific\n> file to use for the rules to apply when formatting, so we can not easily\n> call clang-format with a file specific to its versioned needs.\n> \n> Implement a means of identifying the version of clang-format, and use\n> that information to search for the most recent supportable\n> .clang-format-$(version) file, which can then be symlinked to the fixed\n> file location used by clang format (.clang-format).\n> \n> .clang-format-12: is added with CaseSensitive:true for QT headers\n\nDitto.\n\n> .clang-format-7: is moved from the current .clang-format implementation\n> \n> Signed-off-by: Kieran Bingham \n> \n> ---\n> Note there are few things I dislike about this, hence RFC:\n> \n> - We leave our tree without a .clang-format until checkstyle runs..\n\nA bit annoying, but I think we can live with that. Maybe it's a good way\nto encourage usage of checkstyle.py :-)\n\n> - We run the unlinking and symlinking on every invocation of checkstyle\n\nShouldn't be too costly :-) It's slightly annoying that we can't run\ncheckstyle.py on a read-only directory anymore though, but I don't think\nit's a common use case.\n\n> - The 'autogenerated' file in place of a git tracked one causes\n> issues on rebase. Could be an issue for bisects...?\n\nPossibly, but I don't think we'll often bisect and run checkstyle.py at\nthe same time.\n\n> \n> .clang-format-12 | 169 +++++++++++++++++++++++++++++++\n> .clang-format => .clang-format-7 | 0\n> utils/checkstyle.py | 49 ++++++++-\n\nThe symlink should be added to .gitignore.\n\n> 3 files changed, 217 insertions(+), 1 deletion(-)\n> create mode 100644 .clang-format-12\n> rename .clang-format => .clang-format-7 (100%)\n> \n> diff --git a/.clang-format-12 b/.clang-format-12\n> new file mode 100644\n> index 000000000000..98e9fad472fc\n> --- /dev/null\n> +++ b/.clang-format-12\n> @@ -0,0 +1,169 @@\n> +# SPDX-License-Identifier: GPL-2.0-only\n> +#\n> +# clang-format configuration file. Intended for clang-format >= 12.\n> +#\n> +# For more information, see:\n> +#\n> +# Documentation/process/clang-format.rst\n> +# https://clang.llvm.org/docs/ClangFormat.html\n> +# https://clang.llvm.org/docs/ClangFormatStyleOptions.html\n> +#\n> +---\n> +Language: Cpp\n> +AccessModifierOffset: -8\n> +AlignAfterOpenBracket: Align\n> +AlignConsecutiveAssignments: false\n> +AlignConsecutiveDeclarations: false\n> +AlignEscapedNewlines: Right\n> +AlignOperands: true\n> +AlignTrailingComments: false\n> +AllowAllParametersOfDeclarationOnNextLine: false\n> +AllowShortBlocksOnASingleLine: false\n> +AllowShortCaseLabelsOnASingleLine: false\n> +AllowShortFunctionsOnASingleLine: InlineOnly\n> +AllowShortIfStatementsOnASingleLine: false\n> +AllowShortLoopsOnASingleLine: false\n> +AlwaysBreakAfterDefinitionReturnType: None\n> +AlwaysBreakAfterReturnType: None\n> +AlwaysBreakBeforeMultilineStrings: false\n> +AlwaysBreakTemplateDeclarations: MultiLine\n> +BinPackArguments: true\n> +BinPackParameters: true\n> +BraceWrapping:\n> + AfterClass: true\n> + AfterControlStatement: false\n> + AfterEnum: false\n> + AfterFunction: true\n> + AfterNamespace: false\n> + AfterObjCDeclaration: false\n> + AfterStruct: false\n> + AfterUnion: false\n> + AfterExternBlock: false\n> + BeforeCatch: false\n> + BeforeElse: false\n> + IndentBraces: false\n> + SplitEmptyFunction: true\n> + SplitEmptyRecord: true\n> + SplitEmptyNamespace: true\n> +BreakBeforeBinaryOperators: None\n> +BreakBeforeBraces: Custom\n> +BreakBeforeInheritanceComma: false\n> +BreakInheritanceList: BeforeColon\n> +BreakBeforeTernaryOperators: true\n> +BreakConstructorInitializers: BeforeColon\n> +BreakAfterJavaFieldAnnotations: false\n> +BreakStringLiterals: false\n> +ColumnLimit: 0\n> +CommentPragmas: '^ IWYU pragma:'\n> +CompactNamespaces: false\n> +ConstructorInitializerAllOnOneLineOrOnePerLine: false\n> +ConstructorInitializerIndentWidth: 8\n> +ContinuationIndentWidth: 8\n> +Cpp11BracedListStyle: false\n> +DerivePointerAlignment: false\n> +DisableFormat: false\n> +ExperimentalAutoDetectBinPacking: false\n> +FixNamespaceComments: true\n> +ForEachMacros:\n> + - 'udev_list_entry_foreach'\n> +SortIncludes: true\n> +IncludeBlocks: Regroup\n> +IncludeCategories:\n> + # Headers matching the name of the component are matched automatically.\n> + # Priority 1\n> + # Other library headers (explicit overrides to match before system headers)\n> + - Regex: '(|||||)'\n> + Priority: 9\n> + # Qt includes (match before C++ standard library)\n> + - Regex: ''\n> + Priority: 9\n> + CaseSensitive: true\n> + # Headers in <> with an extension. (+system libraries)\n> + - Regex: '<([A-Za-z0-9\\-_])+\\.h>'\n> + Priority: 2\n> + # System headers\n> + - Regex: ''\n> + Priority: 2\n> + # C++ standard library includes (no extension)\n> + - Regex: '<([A-Za-z0-9\\-_/])+>'\n> + Priority: 2\n> + # Linux headers, as a second group/subset of system headers\n> + - Regex: ''\n> + Priority: 3\n> + # Headers for libcamera Base support\n> + - Regex: ''\n> + Priority: 4\n> + - Regex: ''\n> + Priority: 5\n> + # Public API Headers for libcamera, which are not in a subdir (i.e. ipa/,internal/)\n> + - Regex: ''\n> + Priority: 6\n> + # IPA Interfaces\n> + - Regex: ''\n> + Priority: 7\n> + # libcamera Internal headers in \"\"\n> + - Regex: '\"libcamera/internal/.*\\.h\"'\n> + Priority: 8\n> + # Other libraries headers with one group per library (.h or .hpp)\n> + - Regex: '<.*/.*\\.hp*>'\n> + Priority: 9\n> + # local modular includes \"path/file.h\" (.h or .hpp)\n> + - Regex: '\"(.*/)+.*\\.hp*\"'\n> + Priority: 10\n> + # Other local headers \"file.h\" with extension (.h or .hpp)\n> + - Regex: '\".*.hp*\"'\n> + Priority: 11\n> + # Any unmatched line, separated from the last group\n> + - Regex:\t '\"*\"'\n> + Priority: 100\n> +\n> +IncludeIsMainRegex: '(_test)?$'\n> +IndentCaseLabels: false\n> +IndentPPDirectives: None\n> +IndentWidth: 8\n> +IndentWrappedFunctionNames: false\n> +JavaScriptQuotes: Leave\n> +JavaScriptWrapImports: true\n> +KeepEmptyLinesAtTheStartOfBlocks: false\n> +MacroBlockBegin: ''\n> +MacroBlockEnd: ''\n> +MaxEmptyLinesToKeep: 1\n> +NamespaceIndentation: None\n> +ObjCBinPackProtocolList: Auto\n> +ObjCBlockIndentWidth: 8\n> +ObjCSpaceAfterProperty: true\n> +ObjCSpaceBeforeProtocolList: true\n> +\n> +# Taken from git's rules\n> +PenaltyBreakAssignment: 10\n> +PenaltyBreakBeforeFirstCallParameter: 30\n> +PenaltyBreakComment: 10\n> +PenaltyBreakFirstLessLess: 0\n> +PenaltyBreakString: 10\n> +PenaltyBreakTemplateDeclaration: 10\n> +PenaltyExcessCharacter: 100\n> +PenaltyReturnTypeOnItsOwnLine: 60\n> +\n> +PointerAlignment: Right\n> +ReflowComments: false\n> +SortIncludes: true\n> +SortUsingDeclarations: true\n> +SpaceAfterCStyleCast: false\n> +SpaceAfterTemplateKeyword: false\n> +SpaceBeforeAssignmentOperators: true\n> +SpaceBeforeCpp11BracedList: false\n> +SpaceBeforeCtorInitializerColon: true\n> +SpaceBeforeInheritanceColon: true\n> +SpaceBeforeParens: ControlStatements\n> +SpaceBeforeRangeBasedForLoopColon: true\n> +SpaceInEmptyParentheses: false\n> +SpacesBeforeTrailingComments: 1\n> +SpacesInAngles: false\n> +SpacesInContainerLiterals: false\n> +SpacesInCStyleCastParentheses: false\n> +SpacesInParentheses: false\n> +SpacesInSquareBrackets: false\n> +Standard: Cpp11\n> +TabWidth: 8\n> +UseTab: Always\n> +...\n\nI trust you on this.\n\n> diff --git a/.clang-format b/.clang-format-7\n> similarity index 100%\n> rename from .clang-format\n> rename to .clang-format-7\n> diff --git a/utils/checkstyle.py b/utils/checkstyle.py\n> index ececb46eaacc..279ace8346b4 100755\n> --- a/utils/checkstyle.py\n> +++ b/utils/checkstyle.py\n> @@ -16,6 +16,7 @@\n> import argparse\n> import difflib\n> import fnmatch\n> +import glob\n> import os.path\n> import re\n> import shutil\n> @@ -566,9 +567,55 @@ class Formatter(metaclass=ClassRegistry):\n> return patterns\n> \n> \n> -class CLangFormatter(Formatter):\n> +# Identify the version of clang-format running, and match it against locally\n> +# available versioned configuration files of the form:\n> +# .clang-format-$(version)\n> +#\n> +# The highest supported local configuration file is linked as .clang-format by\n> +# ClangFormatLinker.relink() for use by the ClangFormatter.\n> +#\n> +class ClangFormatLinker():\n\nNo need for parentheses if you don't inherit from another class, but you\ncan inherit from object.\n\nDo we need ClangFormatLinker though ? Couldn't those functions be move\nto ClangFormatter ? Although relinking should probably be done from\nmain(), see below.\n\n> + def version():\n> + version_regex = re.compile('[^0-9]*([0-9]+).*')\n> + ret = subprocess.run(['clang-format', '--version'], stdout=subprocess.PIPE)\n\nWhat happens if clang-format isn't available ? We check for it in\nmain(), but I think this code will run before that.\n\n> + version = ret.stdout.decode('utf-8')\n> + search = version_regex.search(version)\n> + if search:\n> + version = search.group(1)\n> + return int(version)\n> +\n> + # Lowest supported version\n> + return 7\n\nShouldn't this cause an error instead ?\n\n> +\n> + def supported():\n> + version = ClangFormatLinker.version()\n\nThat's a bit of an abuse of the Python class system. Instance methods\nare supposed to be called on an instance and take a self argument. These\nthree functions should be declared as class method I think. This would\nbecome\n\n @classmethod\n def supported(cls):\n version = cls.version()\n\n> +\n> + # Match the highest supported version\n> + clang_files = glob.glob('.clang-format-[0-9]*')\n\nWhen checkstyle.py is run manually, it may be run from a subdirectory of\nthe git tree. You should pass the git top-level directory to relink().\nThis would then call for calling relink() directly from main() instead\nof ClangFormatter.\n\n> + clang_files = sorted(clang_files, reverse=True,\n> + key=lambda s: [int(re.sub(\"[^0-9]\", \"\", s))])\n\nWe use single quotes for strings when possible.\n\n> + for f in clang_files:\n> + file_version = int(re.sub(\"[^0-9]\", \"\", f))\n> + if file_version <= version:\n> + return f\n> +\n> + return None\n> +\n> + def relink():\n> + supported = ClangFormatLinker.supported()\n> + if not supported:\n> + return\n> +\n> + if os.path.exists('.clang-format'):\n> + os.unlink('.clang-format')\n> + os.symlink(supported, '.clang-format')\n\nCould we avoid relinking if the symlink points to the right file already\n?\n\n> +\n> +\n> +class ClangFormatter(Formatter):\n\nCLangFormatter became ClangFormatter. Is it intentional ?\n\n> patterns = ('*.c', '*.cpp', '*.h')\n> \n> + ClangFormatLinker.relink()\n> +\n> @classmethod\n> def format(cls, filename, data):\n> ret = subprocess.run(['clang-format', '-style=file',","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 57438BD87C\n\tfor ;\n\tFri, 27 Aug 2021 15:43:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CF47868932;\n\tFri, 27 Aug 2021 17:43:40 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 58A7960256\n\tfor ;\n\tFri, 27 Aug 2021 17:43:39 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E705C499;\n\tFri, 27 Aug 2021 17:43:38 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"qy5Dftzo\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1630079019;\n\tbh=rXK9A8zNGry9PoCaYxF7kyslhsWNRBYgSSCqRqdVPAE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=qy5DftzooXTH1e98pDEdOjt+0ZsRTqzatGiDG+cQao82ZLBKqh1wx3Y3caTmjR1Kx\n\txvOPRxWjcCo9WbESNjBduzsjyPJxIuLJpu0FdD4uxG8FcAdAPzCPMLMjdfE62sTI/w\n\tx5j4csBDnouAv5DHE+sMkkL4tkgNzvFNCE5n4a0A=","Date":"Fri, 27 Aug 2021 18:43:25 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"","References":"<20210827144753.419929-1-kieran.bingham@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210827144753.419929-1-kieran.bingham@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH] clang-format: Support multiple\n\tversions","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19152,"web_url":"https://patchwork.libcamera.org/comment/19152/","msgid":"<8656e53c-ace0-62cc-fdaa-b9ae2d4f608f@ideasonboard.com>","date":"2021-08-27T16:05:00","subject":"Re: [libcamera-devel] [PATCH] clang-format: Support multiple\n\tversions","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"On 27/08/2021 16:43, Laurent Pinchart wrote:\n> Hi Kieran,\n> \n> Thank you for the patch.\n> \n> On Fri, Aug 27, 2021 at 03:47:53PM +0100, Kieran Bingham wrote:\n>> The .clang-format file can use CaseSensitive on Regex rules for Include\n>> Categories. With this we can distinguish between QT headers which\n> \n> s/QT/Qt/\n> \n>> commence with a 'Q' verses system headers including .\n>>\n>> This requires clang-format-12 to support that rule, and clang-format is\n>> not very amicable to .clang-format files with unsupported entries, and\n>> will fail to run on previous versions if we add that option directly.\n>>\n>> This causes the checkstyle utility to become unusable for users of\n>> clang-format versions prior to 12.\n>>\n>> Unfortunately, clang-format does not provide a way to specify a specific\n>> file to use for the rules to apply when formatting, so we can not easily\n>> call clang-format with a file specific to its versioned needs.\n>>\n>> Implement a means of identifying the version of clang-format, and use\n>> that information to search for the most recent supportable\n>> .clang-format-$(version) file, which can then be symlinked to the fixed\n>> file location used by clang format (.clang-format).\n>>\n>> .clang-format-12: is added with CaseSensitive:true for QT headers\n> \n> Ditto.\n> \n>> .clang-format-7: is moved from the current .clang-format implementation\n>>\n>> Signed-off-by: Kieran Bingham \n>>\n>> ---\n>> Note there are few things I dislike about this, hence RFC:\n>>\n>> - We leave our tree without a .clang-format until checkstyle runs..\n> \n> A bit annoying, but I think we can live with that. Maybe it's a good way\n> to encourage usage of checkstyle.py :-)\n> \n>> - We run the unlinking and symlinking on every invocation of checkstyle\n> \n> Shouldn't be too costly :-) It's slightly annoying that we can't run\n> checkstyle.py on a read-only directory anymore though, but I don't think\n> it's a common use case.\n> \n>> - The 'autogenerated' file in place of a git tracked one causes\n>> issues on rebase. Could be an issue for bisects...?\n> \n> Possibly, but I don't think we'll often bisect and run checkstyle.py at\n> the same time.\n> \n>>\n>> .clang-format-12 | 169 +++++++++++++++++++++++++++++++\n>> .clang-format => .clang-format-7 | 0\n>> utils/checkstyle.py | 49 ++++++++-\n> \n> The symlink should be added to .gitignore.\n> \n>> 3 files changed, 217 insertions(+), 1 deletion(-)\n>> create mode 100644 .clang-format-12\n>> rename .clang-format => .clang-format-7 (100%)\n>>\n>> diff --git a/.clang-format-12 b/.clang-format-12\n>> new file mode 100644\n>> index 000000000000..98e9fad472fc\n>> --- /dev/null\n>> +++ b/.clang-format-12\n>> @@ -0,0 +1,169 @@\n>> +# SPDX-License-Identifier: GPL-2.0-only\n>> +#\n>> +# clang-format configuration file. Intended for clang-format >= 12.\n>> +#\n>> +# For more information, see:\n>> +#\n>> +# Documentation/process/clang-format.rst\n>> +# https://clang.llvm.org/docs/ClangFormat.html\n>> +# https://clang.llvm.org/docs/ClangFormatStyleOptions.html\n>> +#\n>> +---\n>> +Language: Cpp\n>> +AccessModifierOffset: -8\n>> +AlignAfterOpenBracket: Align\n>> +AlignConsecutiveAssignments: false\n>> +AlignConsecutiveDeclarations: false\n>> +AlignEscapedNewlines: Right\n>> +AlignOperands: true\n>> +AlignTrailingComments: false\n>> +AllowAllParametersOfDeclarationOnNextLine: false\n>> +AllowShortBlocksOnASingleLine: false\n>> +AllowShortCaseLabelsOnASingleLine: false\n>> +AllowShortFunctionsOnASingleLine: InlineOnly\n>> +AllowShortIfStatementsOnASingleLine: false\n>> +AllowShortLoopsOnASingleLine: false\n>> +AlwaysBreakAfterDefinitionReturnType: None\n>> +AlwaysBreakAfterReturnType: None\n>> +AlwaysBreakBeforeMultilineStrings: false\n>> +AlwaysBreakTemplateDeclarations: MultiLine\n>> +BinPackArguments: true\n>> +BinPackParameters: true\n>> +BraceWrapping:\n>> + AfterClass: true\n>> + AfterControlStatement: false\n>> + AfterEnum: false\n>> + AfterFunction: true\n>> + AfterNamespace: false\n>> + AfterObjCDeclaration: false\n>> + AfterStruct: false\n>> + AfterUnion: false\n>> + AfterExternBlock: false\n>> + BeforeCatch: false\n>> + BeforeElse: false\n>> + IndentBraces: false\n>> + SplitEmptyFunction: true\n>> + SplitEmptyRecord: true\n>> + SplitEmptyNamespace: true\n>> +BreakBeforeBinaryOperators: None\n>> +BreakBeforeBraces: Custom\n>> +BreakBeforeInheritanceComma: false\n>> +BreakInheritanceList: BeforeColon\n>> +BreakBeforeTernaryOperators: true\n>> +BreakConstructorInitializers: BeforeColon\n>> +BreakAfterJavaFieldAnnotations: false\n>> +BreakStringLiterals: false\n>> +ColumnLimit: 0\n>> +CommentPragmas: '^ IWYU pragma:'\n>> +CompactNamespaces: false\n>> +ConstructorInitializerAllOnOneLineOrOnePerLine: false\n>> +ConstructorInitializerIndentWidth: 8\n>> +ContinuationIndentWidth: 8\n>> +Cpp11BracedListStyle: false\n>> +DerivePointerAlignment: false\n>> +DisableFormat: false\n>> +ExperimentalAutoDetectBinPacking: false\n>> +FixNamespaceComments: true\n>> +ForEachMacros:\n>> + - 'udev_list_entry_foreach'\n>> +SortIncludes: true\n>> +IncludeBlocks: Regroup\n>> +IncludeCategories:\n>> + # Headers matching the name of the component are matched automatically.\n>> + # Priority 1\n>> + # Other library headers (explicit overrides to match before system headers)\n>> + - Regex: '(|||||)'\n>> + Priority: 9\n>> + # Qt includes (match before C++ standard library)\n>> + - Regex: ''\n>> + Priority: 9\n>> + CaseSensitive: true\n>> + # Headers in <> with an extension. (+system libraries)\n>> + - Regex: '<([A-Za-z0-9\\-_])+\\.h>'\n>> + Priority: 2\n>> + # System headers\n>> + - Regex: ''\n>> + Priority: 2\n>> + # C++ standard library includes (no extension)\n>> + - Regex: '<([A-Za-z0-9\\-_/])+>'\n>> + Priority: 2\n>> + # Linux headers, as a second group/subset of system headers\n>> + - Regex: ''\n>> + Priority: 3\n>> + # Headers for libcamera Base support\n>> + - Regex: ''\n>> + Priority: 4\n>> + - Regex: ''\n>> + Priority: 5\n>> + # Public API Headers for libcamera, which are not in a subdir (i.e. ipa/,internal/)\n>> + - Regex: ''\n>> + Priority: 6\n>> + # IPA Interfaces\n>> + - Regex: ''\n>> + Priority: 7\n>> + # libcamera Internal headers in \"\"\n>> + - Regex: '\"libcamera/internal/.*\\.h\"'\n>> + Priority: 8\n>> + # Other libraries headers with one group per library (.h or .hpp)\n>> + - Regex: '<.*/.*\\.hp*>'\n>> + Priority: 9\n>> + # local modular includes \"path/file.h\" (.h or .hpp)\n>> + - Regex: '\"(.*/)+.*\\.hp*\"'\n>> + Priority: 10\n>> + # Other local headers \"file.h\" with extension (.h or .hpp)\n>> + - Regex: '\".*.hp*\"'\n>> + Priority: 11\n>> + # Any unmatched line, separated from the last group\n>> + - Regex:\t '\"*\"'\n>> + Priority: 100\n>> +\n>> +IncludeIsMainRegex: '(_test)?$'\n>> +IndentCaseLabels: false\n>> +IndentPPDirectives: None\n>> +IndentWidth: 8\n>> +IndentWrappedFunctionNames: false\n>> +JavaScriptQuotes: Leave\n>> +JavaScriptWrapImports: true\n>> +KeepEmptyLinesAtTheStartOfBlocks: false\n>> +MacroBlockBegin: ''\n>> +MacroBlockEnd: ''\n>> +MaxEmptyLinesToKeep: 1\n>> +NamespaceIndentation: None\n>> +ObjCBinPackProtocolList: Auto\n>> +ObjCBlockIndentWidth: 8\n>> +ObjCSpaceAfterProperty: true\n>> +ObjCSpaceBeforeProtocolList: true\n>> +\n>> +# Taken from git's rules\n>> +PenaltyBreakAssignment: 10\n>> +PenaltyBreakBeforeFirstCallParameter: 30\n>> +PenaltyBreakComment: 10\n>> +PenaltyBreakFirstLessLess: 0\n>> +PenaltyBreakString: 10\n>> +PenaltyBreakTemplateDeclaration: 10\n>> +PenaltyExcessCharacter: 100\n>> +PenaltyReturnTypeOnItsOwnLine: 60\n>> +\n>> +PointerAlignment: Right\n>> +ReflowComments: false\n>> +SortIncludes: true\n>> +SortUsingDeclarations: true\n>> +SpaceAfterCStyleCast: false\n>> +SpaceAfterTemplateKeyword: false\n>> +SpaceBeforeAssignmentOperators: true\n>> +SpaceBeforeCpp11BracedList: false\n>> +SpaceBeforeCtorInitializerColon: true\n>> +SpaceBeforeInheritanceColon: true\n>> +SpaceBeforeParens: ControlStatements\n>> +SpaceBeforeRangeBasedForLoopColon: true\n>> +SpaceInEmptyParentheses: false\n>> +SpacesBeforeTrailingComments: 1\n>> +SpacesInAngles: false\n>> +SpacesInContainerLiterals: false\n>> +SpacesInCStyleCastParentheses: false\n>> +SpacesInParentheses: false\n>> +SpacesInSquareBrackets: false\n>> +Standard: Cpp11\n>> +TabWidth: 8\n>> +UseTab: Always\n>> +...\n> \n> I trust you on this.\n> \n>> diff --git a/.clang-format b/.clang-format-7\n>> similarity index 100%\n>> rename from .clang-format\n>> rename to .clang-format-7\n>> diff --git a/utils/checkstyle.py b/utils/checkstyle.py\n>> index ececb46eaacc..279ace8346b4 100755\n>> --- a/utils/checkstyle.py\n>> +++ b/utils/checkstyle.py\n>> @@ -16,6 +16,7 @@\n>> import argparse\n>> import difflib\n>> import fnmatch\n>> +import glob\n>> import os.path\n>> import re\n>> import shutil\n>> @@ -566,9 +567,55 @@ class Formatter(metaclass=ClassRegistry):\n>> return patterns\n>> \n>> \n>> -class CLangFormatter(Formatter):\n>> +# Identify the version of clang-format running, and match it against locally\n>> +# available versioned configuration files of the form:\n>> +# .clang-format-$(version)\n>> +#\n>> +# The highest supported local configuration file is linked as .clang-format by\n>> +# ClangFormatLinker.relink() for use by the ClangFormatter.\n>> +#\n>> +class ClangFormatLinker():\n> \n> No need for parentheses if you don't inherit from another class, but you\n> can inherit from object.\n> \n> Do we need ClangFormatLinker though ? Couldn't those functions be move\n> to ClangFormatter ? Although relinking should probably be done from\n> main(), see below.\n\nProbably not, I was just trying to group this code together really, and\nwanted to break out into functions a little to make it clearer.\n\n\n> \n>> + def version():\n>> + version_regex = re.compile('[^0-9]*([0-9]+).*')\n>> + ret = subprocess.run(['clang-format', '--version'], stdout=subprocess.PIPE)\n> \n> What happens if clang-format isn't available ? We check for it in\n> main(), but I think this code will run before that.\n> \n>> + version = ret.stdout.decode('utf-8')\n>> + search = version_regex.search(version)\n>> + if search:\n>> + version = search.group(1)\n>> + return int(version)\n>> +\n>> + # Lowest supported version\n>> + return 7\n> \n> Shouldn't this cause an error instead ?\n\nProbably - we expect there to be a clang-format, and if we can't\ninterrogate it - then it's an issue.\n\n\n> \n>> +\n>> + def supported():\n>> + version = ClangFormatLinker.version()\n> \n> That's a bit of an abuse of the Python class system. Instance methods\n> are supposed to be called on an instance and take a self argument. These\n> three functions should be declared as class method I think. This would\n> become\n> \n> @classmethod\n> def supported(cls):\n> version = cls.version()\n> \n>> +\n>> + # Match the highest supported version\n>> + clang_files = glob.glob('.clang-format-[0-9]*')\n> \n> When checkstyle.py is run manually, it may be run from a subdirectory of\n> the git tree. You should pass the git top-level directory to relink().\n> This would then call for calling relink() directly from main() instead\n> of ClangFormatter.\n> \n>> + clang_files = sorted(clang_files, reverse=True,\n>> + key=lambda s: [int(re.sub(\"[^0-9]\", \"\", s))])\n> \n> We use single quotes for strings when possible.\n\nOk\n\n> \n>> + for f in clang_files:\n>> + file_version = int(re.sub(\"[^0-9]\", \"\", f))\n>> + if file_version <= version:\n>> + return f\n>> +\n>> + return None\n>> +\n>> + def relink():\n>> + supported = ClangFormatLinker.supported()\n>> + if not supported:\n>> + return\n>> +\n>> + if os.path.exists('.clang-format'):\n>> + os.unlink('.clang-format')\n>> + os.symlink(supported, '.clang-format')\n> \n> Could we avoid relinking if the symlink points to the right file already\n> ?\n\nProbably, - this was just the quick and dirty RFC (where I forgot to add\n--rfc to git-format-patch :D)\n\nPresumably os.readlink() will give us something we can work with ...\n\n>> +\n>> +\n>> +class ClangFormatter(Formatter):\n> \n> CLangFormatter became ClangFormatter. Is it intentional ?\n\nYes, but it was supposed to be in a separate patch.\n\nI don't think I've ever seen anyone really call it \"C Lang\" rather than\n\"Clang\"\n\n\n\n> \n>> patterns = ('*.c', '*.cpp', '*.h')\n>> \n>> + ClangFormatLinker.relink()\n>> +\n>> @classmethod\n>> def format(cls, filename, data):\n>> ret = subprocess.run(['clang-format', '-style=file',\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 2E6AFBD87C\n\tfor ;\n\tFri, 27 Aug 2021 16:05:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7D51A6891F;\n\tFri, 27 Aug 2021 18:05:05 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2553660256\n\tfor ;\n\tFri, 27 Aug 2021 18:05:04 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 8839C52F;\n\tFri, 27 Aug 2021 18:05:03 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"uL7NxR23\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1630080303;\n\tbh=k3tDpYWeJB2TgUZP31AZtx6sTgvgzENNFlIwd0+pZi0=;\n\th=From:To:Cc:References:Subject:Date:In-Reply-To:From;\n\tb=uL7NxR23nC6xFsg1ZY06o0yZObJzWtWxgLrdCDoi1Bqv3z6NSpoTUYApckEEEr15q\n\t6uz6ju8WtUnHUQaUWie4/iZbVJRl/MExwDkjbiul8ju/H3Iw6GYw4MEd7ohdJQ0eVQ\n\t3dSL01SVefmVzrnq/XiKSvYdkFvvInpKxwix9dBE=","From":"Kieran Bingham ","To":"Laurent Pinchart ","References":"<20210827144753.419929-1-kieran.bingham@ideasonboard.com>\n\t","Message-ID":"<8656e53c-ace0-62cc-fdaa-b9ae2d4f608f@ideasonboard.com>","Date":"Fri, 27 Aug 2021 17:05:00 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.11.0","MIME-Version":"1.0","In-Reply-To":"","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH] clang-format: Support multiple\n\tversions","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]