[cfe-dev] clang-format: break after 'inline' in function definition
Michael Hofmann
kmhofmann at gmail.com
Sun Dec 21 10:46:19 PST 2014
OK, I now understood the continuation bit. As far as I can see, adding
the respective condition in the
ContinuationIndenter::getNewLineColumn() function should take care of
the indentation level, right? (See diff below.)
However, I still can't get "break after inline" to work properly, even
if I change the condition inside
TokenAnnotator::calculateFormattingInformation() to the one below
(removing the check for tok::comment).
In particular, I experience failure cases with the following code
inline void f(){ // hello
x;
}
template <class T> inline T *f(T &c) { return NULL; }
template <class T> inline T *f(T &c);
where the break after inline won't happen for the first two cases,
where it should. Adding the -debug flag tells me "Could not find a
solution." in UnwrappedLineFormatter::analyzeSolutionSpace(). I'm not
quite sure what's going on there without diving much deeper into the
code base.
It's not terribly important to me personally to prevent breaking after
inline in function *declarations* (again, the keyword should not be
used there according to our coding convention), but it seems more
consistent to so do, since there already is a
'AlwaysBreakAfterDefinitionReturnType' option that breaks the return
type in definitions but not declarations.
It would of course be ideal if the conditions [[~] break after]
[template decl. | inline | return type ] [in declarations |
definitions] would be fully composable, but that appears to be quite
tricky to implement.
Cheers,
Michael
---
[all other changes in the other files as before]
Index: lib/Format/ContinuationIndenter.cpp
===================================================================
--- lib/Format/ContinuationIndenter.cpp (revision 224690)
+++ lib/Format/ContinuationIndenter.cpp (working copy)
@@ -548,7 +548,9 @@
PreviousNonComment->isOneOf(TT_AttributeParen, TT_JavaAnnotation,
TT_LeadingJavaAnnotation))) ||
(!Style.IndentWrappedFunctionNames &&
- NextNonComment->isOneOf(tok::kw_operator, TT_FunctionDeclarationName)))
+ (NextNonComment->isOneOf(tok::kw_operator,
+ TT_FunctionDeclarationName) ||
+ (PreviousNonComment && PreviousNonComment->is(tok::kw_inline)))))
return std::max(State.Stack.back().LastSpace, State.Stack.back().Indent);
if (NextNonComment->is(TT_SelectorName)) {
if (!State.Stack.back().ObjCSelectorNameFound) {
Index: lib/Format/TokenAnnotator.cpp
===================================================================
--- lib/Format/TokenAnnotator.cpp (revision 224690)
+++ lib/Format/TokenAnnotator.cpp (working copy)
@@ -1410,6 +1410,13 @@
// and tok::lbrace.
Current->MustBreakBefore = true;
+ if (Style.AlwaysBreakAfterDefinitionInline &&
+ Current->Previous && Current->Previous->is(tok::kw_inline) &&
+ !Line.Last->is(tok::semi)) // Only for definitions.
+ {
+ Current->MustBreakBefore = true;
+ }
+
Current->CanBreakBefore =
Current->MustBreakBefore || canBreakBefore(Line, *Current);
unsigned ChildSize = 0;
On Sat, Dec 20, 2014 at 9:31 PM, Daniel Jasper <djasper at google.com> wrote:
> 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
More information about the cfe-dev
mailing list