From: "Rémi Bernon" Subject: Easy(er) wine patch formatting with git clang-format. Message-Id: Date: Tue, 22 Sep 2020 00:02:59 +0200 Hi! I had enough of always having small white-space issues in my patches lately, so I've hacked clang-format a bit to make it better handle Wine specific style(s), and make it more readily usable for contributing to Wine, and I'm now sharing it as it could be useful to others. I also discovered the git clang-format helper, which formats source but only keeps the lines that a patch or a commit previously changed, making it easier to get correctly formatted patches without messing with the source code around. Now it's still not completely a perfect fit and there's a few cases where the tool doesn't give very good results, but I find that git clang-format is very well done for hand-picking good style fixes. For instance, you can just add all your changes before committing, then call `git clang-format`, which formats your changes without touching the staged changes. And you can then add the style fixes one by one with `git add -p`. Or, if you already have a commit that you want to check or fixup, run `git clang-format HEAD^` and `git add -p` then `git commit --am`. The patches for clang-format are attached (on llvm source, currently on 45344cf7ac5b848f77825ffa37b0cb3b69b9b07b HEAD), and a small explanation / usage for each one: * PATCH 1/4 adds a new SpacesInFunctionParentheses configuration option to support some Wine-specific style for function calls, where spaces are used but only within parentheses of a function call. There was already some support for spaces around paren, but not specifically for function calls. * PATCH 2/4 introduces a new way to lookup for .clang-format config files. There's already one that looks such files up a source file path, recursively, but as we don't include such files in Wine source, it was hard to maintain locally --I'm a heavy git clean -fdx user. This instead introduces a new -style-dir command-line argument, and a corresponding clangFormat.styleDir git-clang-format configuration parameter, that will replace the current working dir when looking for such files. For instance, I use clangFormat.styleDir = ~/Code/.wine-clang-format.d, which currently contains: /home/rbernon/Code/.wine-clang-format.d/ ├── dlls │   ├── gdi32 │   │   └── .clang-format │   ├── user32 │   │   └── .clang-format │   └── windowscodecs │   └── .clang-format ├── server │   └── .clang-format └── tools └── widl └── .clang-format And clang-format will use the .clang-format that best matches to the source file that is formatted in the wine source tree. For reference and illustration, I have this user32 .clang-format, which works more or less correctly: --- DisableFormat: false SortIncludes: false Language: Cpp StatementMacros: - todo_wine - todo_wine_if TypenameMacros: - MAKE_FUNCPTR - MAKELANGID - LOAD_FUNCPTR - ARRAY_SIZE - debugstr_a - debugstr_an - debugstr_w - debugstr_wn - debugstr_us - debugstr_faceid - debugstr_fontsignature IndentWidth: 4 ColumnLimit: 100 BreakBeforeBraces: Allman AlignAfterOpenBracket: Align AllowAllArgumentsOnNextLine: true AllowShortIfStatementsOnASingleLine: Always AllowShortLoopsOnASingleLine: true BinPackArguments: true BinPackParameters: true SpacesInFunctionParentheses: true PenaltyExcessCharacter: 1 ... I currently abuse StatementMacros and TypenameMacros to tweak the resulting style, as it's still not perfect and the source is also not always consistent already. Of course, this wouldn't be needed if these files could be added to Wine source tree. * PATCH 3/4 and 4/4 are small style mix-ups fixes that occurred too may times already. -- Rémi Bernon From 8fc4c096d6a939c98b4d658d0a7f428f7f76ac6c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9mi=20Bernon?= Date: Thu, 27 Aug 2020 14:53:30 +0200 Subject: [PATCH 1/4] [clang-format] Add new SpacesInFunctionParentheses config. This adds spaces inside parentheses specifically for function calls or alike. --- clang/docs/ClangFormatStyleOptions.rst | 11 +++++++++++ clang/include/clang/Format/Format.h | 12 ++++++++++++ clang/lib/Format/Format.cpp | 3 +++ clang/lib/Format/TokenAnnotator.cpp | 11 +++++++++++ 4 files changed, 37 insertions(+) diff --git a/clang/docs/ClangFormatStyleOptions.rst b/clang/docs/ClangFormatStyleOptions.rst index c35718b5124..ad5528932f7 100644 --- a/clang/docs/ClangFormatStyleOptions.rst +++ b/clang/docs/ClangFormatStyleOptions.rst @@ -2619,6 +2619,17 @@ the configuration (without a prefix: ``Auto``). var arr = [ 1, 2, 3 ]; vs. var arr = [1, 2, 3]; f({a : 1, b : 2, c : 3}); f({a: 1, b: 2, c: 3}); +**SpacesInFunctionParentheses** (``bool``) + If ``true``, spaces will be inserted after ``(`` and before ``)`` of + function calls and declarations. + + .. code-block:: c++ + + true: false: + void f( int ); vs. void f(int); + int g( void ); vs. int g(void); + f( g() ); vs. f(g()); + **SpacesInParentheses** (``bool``) If ``true``, spaces will be inserted after ``(`` and before ``)``. diff --git a/clang/include/clang/Format/Format.h b/clang/include/clang/Format/Format.h index 269eab971a2..b15dac7de26 100644 --- a/clang/include/clang/Format/Format.h +++ b/clang/include/clang/Format/Format.h @@ -2207,6 +2207,17 @@ struct FormatStyle { /// \endcode bool SpacesInCStyleCastParentheses; + /// If ``true``, spaces will be inserted after ``(`` and before ``)`` of + /// function calls and declarations. + /// \code + /// true: false: + /// void f( int ); vs. void f(int); + /// int g( void ); vs. int g(void); + /// f( g() ); vs. f(g()); + /// \endcode + + bool SpacesInFunctionParentheses; + /// If ``true``, spaces will be inserted after ``(`` and before ``)``. /// \code /// true: false: @@ -2434,6 +2445,7 @@ struct FormatStyle { SpacesInConditionalStatement == R.SpacesInConditionalStatement && SpacesInContainerLiterals == R.SpacesInContainerLiterals && SpacesInCStyleCastParentheses == R.SpacesInCStyleCastParentheses && + SpacesInFunctionParentheses == R.SpacesInFunctionParentheses && SpacesInParentheses == R.SpacesInParentheses && SpacesInSquareBrackets == R.SpacesInSquareBrackets && SpaceBeforeSquareBrackets == R.SpaceBeforeSquareBrackets && diff --git a/clang/lib/Format/Format.cpp b/clang/lib/Format/Format.cpp index 8c1d7c90e02..6b44f1404a1 100644 --- a/clang/lib/Format/Format.cpp +++ b/clang/lib/Format/Format.cpp @@ -600,6 +600,8 @@ template <> struct MappingTraits { Style.SpacesInContainerLiterals); IO.mapOptional("SpacesInCStyleCastParentheses", Style.SpacesInCStyleCastParentheses); + IO.mapOptional("SpacesInFunctionParentheses", + Style.SpacesInFunctionParentheses); IO.mapOptional("SpacesInParentheses", Style.SpacesInParentheses); IO.mapOptional("SpacesInSquareBrackets", Style.SpacesInSquareBrackets); IO.mapOptional("SpaceBeforeSquareBrackets", @@ -914,6 +916,7 @@ FormatStyle getLLVMStyle(FormatStyle::LanguageKind Language) { LLVMStyle.UseCRLF = false; LLVMStyle.UseTab = FormatStyle::UT_Never; LLVMStyle.ReflowComments = true; + LLVMStyle.SpacesInFunctionParentheses = false; LLVMStyle.SpacesInParentheses = false; LLVMStyle.SpacesInSquareBrackets = false; LLVMStyle.SpaceInEmptyBlock = false; diff --git a/clang/lib/Format/TokenAnnotator.cpp b/clang/lib/Format/TokenAnnotator.cpp index 11acb597aa4..f9059ad787b 100644 --- a/clang/lib/Format/TokenAnnotator.cpp +++ b/clang/lib/Format/TokenAnnotator.cpp @@ -2758,6 +2758,17 @@ bool TokenAnnotator::spaceRequiredBetween(const AnnotatedLine &Line, (Left.is(tok::l_brace) && Left.isNot(BK_Block) && Right.is(tok::r_brace) && Right.isNot(BK_Block))) return Style.SpaceInEmptyParentheses; + if (Style.SpacesInFunctionParentheses) { + if (Left.is(tok::l_paren) && Left.Previous && + Left.Previous->is(tok::identifier) && + !Left.Previous->is(TT_TypenameMacro)) + return true; + if (Right.is(tok::r_paren) && Right.MatchingParen && + Right.MatchingParen->Previous && + Right.MatchingParen->Previous->is(tok::identifier) && + !Right.MatchingParen->Previous->is(TT_TypenameMacro)) + return true; + } if (Style.SpacesInConditionalStatement) { if (Left.is(tok::l_paren) && Left.Previous && isKeywordWithCondition(*Left.Previous)) -- 2.28.0 From caa9661cf3d565693242edf6a2fe225f7949ed72 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9mi=20Bernon?= Date: Thu, 27 Aug 2020 14:54:37 +0200 Subject: [PATCH 2/4] [clang-format] Add -style-dir command-line argument. This makes it possible to store the .clang-format in a directory tree alongside a project, looking up style files relative to this tree instead of the current working directory. --- clang/tools/clang-format/ClangFormat.cpp | 12 ++++++++++-- clang/tools/clang-format/git-clang-format | 23 +++++++++++++++++------ 2 files changed, 27 insertions(+), 8 deletions(-) diff --git a/clang/tools/clang-format/ClangFormat.cpp b/clang/tools/clang-format/ClangFormat.cpp index aa40bab52df..dfe62a7c5c7 100644 --- a/clang/tools/clang-format/ClangFormat.cpp +++ b/clang/tools/clang-format/ClangFormat.cpp @@ -63,6 +63,11 @@ static cl::opt Style("style", cl::desc(clang::format::StyleOptionHelpDescription), cl::init(clang::format::DefaultFormatStyle), cl::cat(ClangFormatCategory)); +static cl::opt + StyleDir("style-dir", + cl::desc("The directory where to look for matching style files\n" + "instead of the current working directory.\n"), + cl::init("."), cl::cat(ClangFormatCategory)); static cl::opt FallbackStyle("fallback-style", cl::desc("The name of the predefined style used as a\n" @@ -377,8 +382,11 @@ static bool format(StringRef FileName) { return true; } - llvm::Expected FormatStyle = - getStyle(Style, AssumedFileName, FallbackStyle, Code->getBuffer()); + auto AbsoluteAssumedFileName = (StyleDir + "/" + AssumedFileName).str(); + if (AssumedFileName[0] != '/') + AssumedFileName = AbsoluteAssumedFileName; + llvm::Expected FormatStyle = getStyle( + Style, AbsoluteAssumedFileName, FallbackStyle, Code->getBuffer()); if (!FormatStyle) { llvm::errs() << llvm::toString(FormatStyle.takeError()) << "\n"; return true; diff --git a/clang/tools/clang-format/git-clang-format b/clang/tools/clang-format/git-clang-format index e4dc4cbc1dc..4b95d446fc8 100755 --- a/clang/tools/clang-format/git-clang-format +++ b/clang/tools/clang-format/git-clang-format @@ -47,6 +47,7 @@ The following git-config settings set the default of the corresponding option: clangFormat.commit clangFormat.extensions clangFormat.style + clangFormat.styleDir ''' # Name of the temporary index file in which save the output of clang-format. @@ -112,6 +113,9 @@ def main(): p.add_argument('--style', default=config.get('clangformat.style', None), help='passed to clang-format'), + p.add_argument('--style-dir', + default=config.get('clangformat.styledir', None), + help='passed to clang-format'), p.add_argument('-v', '--verbose', action='count', default=0, help='print extra information') # We gather all the remaining positional arguments into 'args' since we need @@ -159,12 +163,14 @@ def main(): new_tree = run_clang_format_and_save_to_tree(changed_lines, revision=commits[1], binary=opts.binary, - style=opts.style) + style=opts.style, + style_dir=opts.style_dir) else: old_tree = create_tree_from_workdir(changed_lines) new_tree = run_clang_format_and_save_to_tree(changed_lines, binary=opts.binary, - style=opts.style) + style=opts.style, + style_dir=opts.style_dir) if opts.verbose >= 1: print('old tree: %s' % old_tree) print('new tree: %s' % new_tree) @@ -351,7 +357,8 @@ def create_tree_from_workdir(filenames): def run_clang_format_and_save_to_tree(changed_lines, revision=None, - binary='clang-format', style=None): + binary='clang-format', style=None, + style_dir=None): """Run clang-format on each file and save the result to a git tree. Returns the object ID (SHA-1) of the created tree.""" @@ -378,7 +385,8 @@ def run_clang_format_and_save_to_tree(changed_lines, revision=None, blob_id = clang_format_to_blob(filename, line_ranges, revision=revision, binary=binary, - style=style) + style=style, + style_dir=style_dir) yield '%s %s\t%s' % (mode, blob_id, filename) return create_tree(index_info_generator(), '--index-info') @@ -404,16 +412,19 @@ def create_tree(input_lines, mode): def clang_format_to_blob(filename, line_ranges, revision=None, - binary='clang-format', style=None): + binary='clang-format', style=None, + style_dir=None): """Run clang-format on the given file and save the result to a git blob. Runs on the file in `revision` if not None, or on the file in the working directory if `revision` is None. Returns the object ID (SHA-1) of the created blob.""" - clang_format_cmd = [binary] + clang_format_cmd = [os.path.expanduser(binary)] if style: clang_format_cmd.extend(['-style='+style]) + if style_dir: + clang_format_cmd.extend(['-style-dir='+os.path.expanduser(style_dir)]) clang_format_cmd.extend([ '-lines=%s:%s' % (start_line, start_line+line_count-1) for start_line, line_count in line_ranges]) -- 2.28.0 From 70c4a08f67482a6528f9f26c759d3fece900f6a5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9mi=20Bernon?= Date: Thu, 27 Aug 2020 14:56:07 +0200 Subject: [PATCH 3/4] [clang-format] Don't break every element of a long nested braced initializer. When AlignAfterOpenBracket = Align is used, that looks horrible. --- clang/lib/Format/ContinuationIndenter.cpp | 3 +++ clang/lib/Format/TokenAnnotator.cpp | 2 +- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/clang/lib/Format/ContinuationIndenter.cpp b/clang/lib/Format/ContinuationIndenter.cpp index d99107cb8b2..ed4c2af4144 100644 --- a/clang/lib/Format/ContinuationIndenter.cpp +++ b/clang/lib/Format/ContinuationIndenter.cpp @@ -645,6 +645,9 @@ void ContinuationIndenter::addTokenOnCurrentLine(LineState &State, bool DryRun, if (Previous.is(TT_TemplateString) && Previous.opensScope()) State.Stack.back().NoLineBreak = true; + if (Style.AlignAfterOpenBracket == FormatStyle::BAS_Align && + Previous.is(BK_BracedInit) && Current.NestingLevel != 0) + State.Stack.back().NoLineBreak = true; if (Style.AlignAfterOpenBracket != FormatStyle::BAS_DontAlign && !State.Stack.back().IsCSharpGenericTypeConstraint && Previous.opensScope() && Previous.isNot(TT_ObjCMethodExpr) && diff --git a/clang/lib/Format/TokenAnnotator.cpp b/clang/lib/Format/TokenAnnotator.cpp index f9059ad787b..531b297036d 100644 --- a/clang/lib/Format/TokenAnnotator.cpp +++ b/clang/lib/Format/TokenAnnotator.cpp @@ -2681,7 +2681,7 @@ unsigned TokenAnnotator::splitPenalty(const AnnotatedLine &Line, if (Left.is(TT_TemplateOpener)) return 100; if (Left.opensScope()) { - if (Style.AlignAfterOpenBracket == FormatStyle::BAS_DontAlign) + if (Style.AlignAfterOpenBracket != FormatStyle::BAS_AlwaysBreak) return 0; if (Left.is(tok::l_brace) && !Style.Cpp11BracedListStyle) return 19; -- 2.28.0 From d7f09e704ecff898683086127d7c347949d3bba2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9mi=20Bernon?= Date: Mon, 21 Sep 2020 23:24:43 +0200 Subject: [PATCH 4/4] [clang-format] Also allow one-line else if AllowShortIfStatementsOnASingleLine. Fixing this: if (x) return 1; else return 0; Being incorrectly reformatted as: if (x) return 1; else return 0; --- clang/lib/Format/UnwrappedLineFormatter.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/clang/lib/Format/UnwrappedLineFormatter.cpp b/clang/lib/Format/UnwrappedLineFormatter.cpp index d8f718306a4..120a07fccd7 100644 --- a/clang/lib/Format/UnwrappedLineFormatter.cpp +++ b/clang/lib/Format/UnwrappedLineFormatter.cpp @@ -387,7 +387,7 @@ private: } return MergedLines; } - if (TheLine->First->is(tok::kw_if)) { + if (TheLine->First->is(tok::kw_if) || TheLine->First->is(tok::kw_else)) { return Style.AllowShortIfStatementsOnASingleLine ? tryMergeSimpleControlStatement(I, E, Limit) : 0; @@ -437,7 +437,8 @@ private: return 0; Limit = limitConsideringMacros(I + 1, E, Limit); AnnotatedLine &Line = **I; - if (!Line.First->is(tok::kw_do) && Line.Last->isNot(tok::r_paren)) + if (!Line.First->is(tok::kw_do) && Line.Last->isNot(tok::r_paren) && + Line.Last->isNot(tok::kw_else)) return 0; // Only merge do while if do is the only statement on the line. if (Line.First->is(tok::kw_do) && !Line.Last->is(tok::kw_do)) -- 2.28.0