[cfe-dev] clang-format: break after 'inline' in function definition
Daniel Jasper
djasper at google.com
Sat Dec 20 12:31:42 PST 2014
By default, a line break somewhere within a statement or a declaration is a
continuation and is indented by whatever is configured in
ContinuationIndentWidth. Not indenting when wrapping after the template
declaration or return type is special cased. Take look at the function
getNewLineColumn in ContinuationIndenter.cpp, the special case here is the
if at line 546.
As for the patch itself, how important is it not to break after inline in
declarations? It seems to add unnecessary complexity and is wrong as
implemented (won't break in "inline void f() { // comment").
Cheers,
Daniel
On Sat, Dec 20, 2014 at 6:51 PM, Michael Hofmann <kmhofmann at gmail.com>
wrote:
>
> I am trying to add a style option to clang-format, in order to get one
> step closer to mapping our coding style to a .clang-format file.
> Our guidelines mandate that for function definitions and non-member
> function declarations, the following elements (if they exist) need to
> be on a separate line each:
> - template specification
> - inline keyword [only legal in function definitions]
> - function return type
> - function name (and arguments, potentially on separate lines)
>
> This is to increase discoverability of the return type as well as the
> function name. Consider a complex function definition such as:
> template <typename BidirIt, typename IndexType>
> inline
> std::iterator_traits<BidirIt>::value_type
> Foo<ScalarType>::bar(BidirIt begin, BidirIt end, IndexType index)
> { ...
>
> With the currently available clang-format options
> AlwaysBreakAfterDefinitionReturnType: true
> AlwaysBreakTemplateDeclarations: true
> one can get as far as:
>
> template <typename BidirIt, typename IndexType>
> inline std::iterator_traits<BidirIt>::value_type
> Foo<ScalarType>::bar(BidirIt begin, BidirIt end, IndexType index)
> { ...
>
> but there is no good reason why the inline keyword should reside on
> the same line as the return type (they are not logically linked in any
> way). Hence our guideline to have it on a separate line, which makes
> spotting the return type easier, too.
>
> I have tried to add a new boolean option
> 'AlwaysBreakAfterDefinitionInline', somewhat mimicking the
> implementation of 'AlwaysBreakAfterDefinitionReturnType'. The
> intention is to break after the 'inline' keyword in a definition, but
> not in a declaration (where the keyword is not supposed to be anyway).
> See the patch below, which appears to do the right thing using our
> coding conventions.
>
> However, the new unit test, which I added as a copy-&-paste from the
> AlwaysBreakAfterDefinitionReturnType test, is broken. clang-format (in
> combination with LLVM style) seems to indent the lines right after the
> inline keyword, which gives the following error:
>
> Value of: format(test::messUp(Code), Style)
> Actual: "inline\n const char *\n f(void) {\n return
> \"\";\n}\ninline const char *bar(void);\n"
> Expected: Code.str()
> Which is: "inline\nconst char *f(void) {\n return \"\";\n}\ninline
> const char *bar(void);\n"
> ...
>
> Since this is my first excursion into the clang-format codebase, it is
> not clear to me how this line breaking is linked to indentation.
> Can someone help me out there? Or ideally implement the option correctly?
> :)
>
> Thanks,
> Michael
>
> ---
>
> Index: include/clang/Format/Format.h
> ===================================================================
> --- include/clang/Format/Format.h (revision 224222)
> +++ include/clang/Format/Format.h (working copy)
> @@ -275,6 +275,10 @@
> /// template declaration.
> bool AlwaysBreakTemplateDeclarations;
>
> + /// \brief If \c true, always break after the inline keyword of a
> function
> + /// definition.
> + bool AlwaysBreakAfterDefinitionInline;
> +
> /// \brief If \c true, always break before multiline string literals.
> bool AlwaysBreakBeforeMultilineStrings;
>
> @@ -426,6 +430,8 @@
> R.AlwaysBreakAfterDefinitionReturnType &&
> AlwaysBreakTemplateDeclarations ==
> R.AlwaysBreakTemplateDeclarations &&
> + AlwaysBreakAfterDefinitionInline ==
> + R.AlwaysBreakAfterDefinitionInline &&
> AlwaysBreakBeforeMultilineStrings ==
> R.AlwaysBreakBeforeMultilineStrings &&
> BinPackParameters == R.BinPackParameters &&
> Index: lib/Format/Format.cpp
> ===================================================================
> --- lib/Format/Format.cpp (revision 224222)
> +++ lib/Format/Format.cpp (working copy)
> @@ -192,6 +192,8 @@
> Style.AlwaysBreakAfterDefinitionReturnType);
> IO.mapOptional("AlwaysBreakTemplateDeclarations",
> Style.AlwaysBreakTemplateDeclarations);
> + IO.mapOptional("AlwaysBreakAfterDefinitionInline",
> + Style.AlwaysBreakAfterDefinitionInline);
> IO.mapOptional("AlwaysBreakBeforeMultilineStrings",
> Style.AlwaysBreakBeforeMultilineStrings);
> IO.mapOptional("BreakBeforeBinaryOperators",
> @@ -340,6 +342,7 @@
> LLVMStyle.AlwaysBreakAfterDefinitionReturnType = false;
> LLVMStyle.AlwaysBreakBeforeMultilineStrings = false;
> LLVMStyle.AlwaysBreakTemplateDeclarations = false;
> + LLVMStyle.AlwaysBreakAfterDefinitionInline = false;
> LLVMStyle.BinPackParameters = true;
> LLVMStyle.BinPackArguments = true;
> LLVMStyle.BreakBeforeBinaryOperators = FormatStyle::BOS_None;
> Index: lib/Format/TokenAnnotator.cpp
> ===================================================================
> --- lib/Format/TokenAnnotator.cpp (revision 224222)
> +++ lib/Format/TokenAnnotator.cpp (working copy)
> @@ -1409,6 +1409,11 @@
> // and tok::lbrace.
> Current->MustBreakBefore = true;
>
> + if (Style.AlwaysBreakAfterDefinitionInline &&
> + Current->Previous && Current->Previous->is(tok::kw_inline) &&
> + !Line.Last->isOneOf(tok::semi, tok::comment)) // Only for
> definitions.
> + Current->MustBreakBefore = true;
> +
> Current->CanBreakBefore =
> Current->MustBreakBefore || canBreakBefore(Line, *Current);
> unsigned ChildSize = 0;
> Index: unittests/Format/FormatTest.cpp
> ===================================================================
> --- unittests/Format/FormatTest.cpp (revision 224222)
> +++ unittests/Format/FormatTest.cpp (working copy)
> @@ -4503,6 +4503,40 @@
> AfterType);
> }
>
> +TEST_F(FormatTest, AlwaysBreakAfterDefinitionInline) {
> + FormatStyle AfterType = getLLVMStyle();
> + AfterType.AlwaysBreakAfterDefinitionInline = true;
> + verifyFormat("inline\n" // Break here.
> + "const char *f(void) {\n"
> + " return \"\";\n"
> + "}\n"
> + "inline const char *bar(void);\n", // No break here.
> + AfterType);
> + verifyFormat("template <class T>\n"
> + "inline\n" // Break here.
> + "T *f(T &c) {\n"
> + " return NULL;\n"
> + "}\n"
> + "template <class T> inline T *f(T &c);\n", // No break
> here.
> + AfterType);
> + AfterType.BreakBeforeBraces = FormatStyle::BS_Stroustrup;
> + verifyFormat("inline\n" // Break here.
> + "const char *f(void)\n"
> + "{\n"
> + " return \"\";\n"
> + "}\n"
> + "inline const char *bar(void);\n", // No break here.
> + AfterType);
> + verifyFormat("template <class T>\n"
> + "inline\n"
> + "T *f(T &c)\n" // Break here.
> + "{\n"
> + " return NULL;\n"
> + "}\n"
> + "template <class T> inline T *f(T &c);\n", // No break
> here.
> + AfterType);
> +}
> +
> TEST_F(FormatTest, AlwaysBreakBeforeMultilineStrings) {
> FormatStyle NoBreak = getLLVMStyle();
> NoBreak.AlwaysBreakBeforeMultilineStrings = false;
> @@ -8674,6 +8708,7 @@
> CHECK_PARSE_BOOL(AllowShortIfStatementsOnASingleLine);
> CHECK_PARSE_BOOL(AllowShortLoopsOnASingleLine);
> CHECK_PARSE_BOOL(AlwaysBreakAfterDefinitionReturnType);
> + CHECK_PARSE_BOOL(AlwaysBreakAfterDefinitionInline);
> CHECK_PARSE_BOOL(AlwaysBreakTemplateDeclarations);
> CHECK_PARSE_BOOL(BinPackParameters);
> CHECK_PARSE_BOOL(BinPackArguments);
> _______________________________________________
> cfe-dev mailing list
> cfe-dev at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20141220/a57d087b/attachment.html>
More information about the cfe-dev
mailing list