r263709 - clang-format: Slightly weaken AlignAfterOpenBracket=AlwaysBreak.
Jean-philippe Dufraigne via cfe-commits
cfe-commits at lists.llvm.org
Sun Mar 27 11:16:08 PDT 2016
Hi Daniel,
Thanks a lot for your answer and clarifications.
Sorry for the long answer, TLDR at the beginning (and end for code
examples).
To answer your 2 questions immediately:
- It happens a lot more than I thought, about 10% of the few thousands file
I checked had instances of this.
- Yes, there is a significant readability disadvantage as these functions
become a special case that needs to be read differently from all the other
functions.
With consistent AlwaysBreak, it is obvious from the shape of the call how
to read it (horizontally or diagonally).
Now, it is not longer obvious and it impact readability mostly for these
functions, but also a bit for the other functions since it is not clear if
they are one of the special case or not.
(The eye needs to jump at the end and then back to parse the arguments)
(This ls less/not of an issue for lambda and small functions)
My conclusion:
I really think an option should provide a consistent break but I'm really
not sure it should be a new option.
If I manage to convince you through the examples, that formatting all
function consistently will actually improve overall readability more
generally, I think AlwaysBreak is just fine as it was.
I'm happy to help with coding or writing tests for whatever decision you
make.
Examples:
I've pasted / attached the code examples that I saw, some many many times
(with .clang-format file).
You'll be able to see how before things were more consistent and now things
are moved to unexpected places depending on the call.
Moreover the benefit is very limited for us, because gaining 1 line does
not seem to add to our readability, and the indentation saved (most often 1
(4 spaces)) does not lead to better or different arguments formatting in
most cases. (less than 10 where arguments where improved in the hundreds of
cases I've looked at).
Methodology:
I wanted to make sure that I really understood the problem before coming
back to you:
- I've done a full reformat of 2 of our projects first with snapshot
r260967, then with snapshot r264047 (with the new change).
That is few thousands of cpp/h files, and about 10% were modified when
running the updated format.
- I spent more hours than I should have yesterday just scrolling through
and taking notes in a 30'000 line diff of the impact of the update (using
10 lines of context), so I think I've got a good handle on the issue now.
Other comments:
- It is possible that with some other configuration you used, this change
does improves things without the same draw back, I do not know, but your
commit comment example did not look more readable for me since I am used to
the style.
- lambda break would be nice and more what we would have expected, but they
are somewhat a different thing and less of a problem.
- Very small function break occurs not that often and are weird to format
either way. They are also less of an issue because they are small so
everything is mostly where the eye is looking at (no need for the eye to
jump toward the end of the line and back).
- I also think the new option as you described could have some trickiness,
it may also impact things like 'if' if we are not careful, and it is also
another maintenance burden.
Other notes / detailed explanations:
1 - With a consistent always break, there are 2 places you have too look
for, and the one your need is obvious from the shape of the code:
- Either it is multi-line, and 1st argument is described at the
indentation => Scan function call diagonally
- Or it is a single line call and the 1st argument is described after the
'(' => Scan function horizontally
With this new formatting, the 1st argument may also start to be described
after the '(' and then continue at the indentation.
It is no longer possible to scan diagonally, and that really apply for any
functions as it is not obvious from the shape of the call.
2 - It may be because of our other style options, but there are very few
(less than 10 in the many hundreds) where the result could be said to be
better, on its own:
This happens, because we most often only gain a new line (not really an
improvement for us) and only one indentation (4 spaces), and the odds that
the argument is too big by between 1 and 4 spaces is not so great:
So the net result is that the argument are shifted 4 spaces to the left
without really any improvements.
3- There is one unrelated bug with unstable comment re flowing in macros
that I found as a result and will try to raise it.
There was also some minor changes in the way few 'else if' were
formatted but this is not the main matter.
There was also an issue with formatting a template call with explicit
types as a function argument indented strangely.
Cheers,
Jean-Philippe
NewTest.cpp (also attached, contains both code reformatted with r26096 and
with r264047):
You can see how before things were more consistent and now things are moved
to unexpected places depending on the call.
class TestClass1
{
//
// Before:
//
typedef boost::multi_index_container<
DataRecordPtr,
mi::indexed_by<
mi::ordered_unique<
mi::const_mem_fun<
Type00000001,
Type00000000002,
&Type00000001::Fct > >,
mi::ordered_non_unique<
mi::const_mem_fun<
Type000003,
Type00004,
&Type000003::Fct0002 > > > >
TypedefName0000;
typedef boost::function<
StaticClass01::TypeOfReturnValue0001( ParamType1* ) >
TypedefName0000000001;
typedef boost::function<
std::wstring(
int,
ClassName000001::Value00000001,
const std::wstring& ) >
TypedefName000000000002;
//
// After r264047:
//
typedef boost::multi_index_container<
DataRecordPtr,
mi::indexed_by<
mi::ordered_unique< mi::const_mem_fun<
Type00000001,
Type00000000002,
&Type00000001::Fct > >,
mi::ordered_non_unique< mi::const_mem_fun<
Type000003,
Type00004,
&Type000003::Fct0002 > > > >
TypedefName0000;
typedef boost::function< StaticClass01::TypeOfReturnValue0001(
ParamType1* ) >
TypedefName0000000001;
typedef boost::function< std::wstring(
int,
ClassName000001::Value00000001,
const std::wstring& ) >
TypedefName000000000002;
};
void Test1()
{
//
// Before:
//
connRegist.Add(
member1->ConnectSignal00001(
boost::bind( &ThisClassNme::Callback00001, this, _1 ) ) );
connRegist.Add(
member1->ConnectSignal00002(
boost::bind( &ThisClassNme::Callback000000002, this ) ) );
connRegist.Add(
member1->ConnectSignal00003(
boost::bind( &ThisClassNme::Callback0000003, this, _1 ), true )
);
connRegist.Add(
member1->ConnectSignal00004(
boost::bind(
&ThisClassNme::Callback00000000000000000000000004, this ) )
);
connRegist.Add(
member1->ConnectSignal00003(
boost::bind(
&ThisClassNme::Callback00000000000000000000000005, this, _1
),
true ) );
//
// After r264047:
//
connRegist.Add( member1->ConnectSignal00001(
boost::bind( &ThisClassNme::Callback00001, this, _1 ) ) );
connRegist.Add( member1->ConnectSignal00002(
boost::bind( &ThisClassNme::Callback000000002, this ) ) );
connRegist.Add( member1->ConnectSignal00003(
boost::bind( &ThisClassNme::Callback0000003, this, _1 ), true ) );
connRegist.Add( member1->ConnectSignal00004( boost::bind(
&ThisClassNme::Callback00000000000000000000000004, this ) ) );
connRegist.Add( member1->ConnectSignal00003(
boost::bind(
&ThisClassNme::Callback00000000000000000000000005, this, _1 ),
true ) );
}
void Test1()
{
//
// Before:
//
CPPUNIT_ASSERT_NO_THROW(
object.member00000001->Function1( L"string0000001", L"string002" )
);
// The different one after:
CPPUNIT_ASSERT(
object.member0002->Function000002(
object.member00000001->Function000000002() ) );
CPPUNIT_ASSERT_THROW(
object.member00000001->Function1( L"strin", L"string03" ),
ExpecException );
//
// After r264047:
//
CPPUNIT_ASSERT_NO_THROW(
object.member00000001->Function1( L"string0000001", L"string002" )
);
// The different one after:
CPPUNIT_ASSERT( object.member0002->Function000002(
object.member00000001->Function000000002() ) );
CPPUNIT_ASSERT_THROW(
object.member00000001->Function1( L"strin", L"string03" ),
ExpecException );
}
void Test2()
{
//
// Before:
//
boost::scoped_ptr< Type01 > obj001(
new Type01(
nsp::nmspc::ServiceInfoPtr(
new nsp::nmspc::TypeInfo001(
key,
key,
nsp::Conver< std::string >( objectName ),
nsp::nmspc::nmspace::Value000001,
nsp::nmspc::nmspace::Value002 ) ),
extraArg001 ) );
boost::scoped_ptr< Type01 > obj002(
new Type01(
nsp::nmspc::TypeInfo001Ptr(
new nsp::nmspc::TypeInfo001(
key,
key,
nsp::Conver< std::string >( objectName ),
nsp::nmspc::nmspace::Value000001,
nsp::nmspc::nmspace::Value002 ) ) ) );
//
// After r264047:
//
boost::scoped_ptr< Type01 > obj001( new Type01(
nsp::nmspc::ServiceInfoPtr( new nsp::nmspc::TypeInfo001(
key,
key,
nsp::Conver< std::string >( objectName ),
nsp::nmspc::nmspace::Value000001,
nsp::nmspc::nmspace::Value002 ) ),
extraArg001 ) );
boost::scoped_ptr< Type01 > obj002(
new Type01( nsp::nmspc::TypeInfo001Ptr( new nsp::nmspc::TypeInfo001(
key,
key,
nsp::Conver< std::string >( objectName ),
nsp::nmspc::nmspace::Value000001,
nsp::nmspc::nmspace::Value002 ) ) ) );
}
void Test3
{
{
{
//
// Before:
//
memberObject0000000001.reset(
new nmspc::namespc::scoped_register01(
object00001->ConnectToSomeEvent0001(
boost::bind(
&namespc00001::ClassName0000001::FunctionName00001,
this,
_1 ) ) ) );
//
// After r264047:
//
memberObject0000000001.reset( new
nmspc::namespc::scoped_register01(
object00001->ConnectToSomeEvent0001( boost::bind(
&namespc00001::ClassName0000001::FunctionName00001,
this,
_1 ) ) ) );
}
}
}
2016-03-24 20:20 GMT+00:00 Daniel Jasper <djasper at google.com>:
> Hi Jean-Philippe,
>
> first off, sorry that this is causing you trouble.
>
> I am always happy to reconsider. However, I do think that this is a good
> change based on hundreds of examples I have looked at. Obviously, this is
> subjective and I won't even start to make an argument on whether this is
> wasted or not.
>
> We could add a separate option, but it is not quite as easy. AlwaysBreak
> has never been *always* break. There have always been exceptions. Two
> immediately spring to mind:
> 1. When the function name (plus open parenthesis) is shorter or equal to
> the ContinuationIndentWidth.
> 2. For nested blocks such as lambdas, at least in some of the cases.
>
> Both are also about wasting space. I think single argument function calls
> are closely related to those two. So, we could add an "StrictAlwaysBreak"
> mode and then also not have the two exemptions above, but I am not sure it
> is worth it.
>
> How often do these cases occur in your codebase? Do you really feel like
> there is a significant readability disadvantage?
>
> Cheers,
> Daniel
>
> On Sun, Mar 20, 2016 at 12:00 PM, Jean-philippe Dufraigne <
> j.dufraigne at gmail.com> wrote:
>
>> Hi Daniel,
>>
>> I'm not sure I understand the logic behind modifying 'AlwaysBreak' to be
>> 'AlwaysBreakExceptForSingleArgument'.
>>
>> AlwaysBreak is always going to lead to "wasted space".
>> Some consider consistently break to not be a waste but actually
>> contribute to readability, it seems that was what 'AlwaysBreak' was about.
>>
>> Would you reconsider this change, or make it an option different from
>> 'AlwaysBreak'?
>>
>>
>>
>> We just completed the roll out clang-format at my company.
>> We are using: AlignAfterOpenBracket: AlwaysBreak
>> We tuned the settings to have a style that does not clash too much with
>> our original style, and that will look like a regression for us.
>>
>> Kind regards,
>> Jean-Philippe.
>>
>>
>> 2016-03-17 12:00 GMT+00:00 Daniel Jasper via cfe-commits <
>> cfe-commits at lists.llvm.org>:
>>
>>> Author: djasper
>>> Date: Thu Mar 17 07:00:22 2016
>>> New Revision: 263709
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=263709&view=rev
>>> Log:
>>> clang-format: Slightly weaken AlignAfterOpenBracket=AlwaysBreak.
>>>
>>> If a call takes a single argument, using AlwaysBreak can lead to lots
>>> of wasted lines and additional indentation without improving the
>>> readability in a significant way.
>>>
>>> Before:
>>> caaaaaaaaaaaall(
>>> caaaaaaaaaaaall(
>>> caaaaaaaaaaaall(
>>> caaaaaaaaaaaaaaaaaaaaaaall(aaaaaaaaaaaaaa, aaaaaaaaa))));
>>>
>>> After:
>>> caaaaaaaaaaaall(caaaaaaaaaaaall(caaaaaaaaaaaall(
>>> caaaaaaaaaaaaaaaaaaaaaaall(aaaaaaaaaaaaaa, aaaaaaaaa))));
>>>
>>> Modified:
>>> cfe/trunk/lib/Format/ContinuationIndenter.cpp
>>> cfe/trunk/unittests/Format/FormatTest.cpp
>>>
>>> Modified: cfe/trunk/lib/Format/ContinuationIndenter.cpp
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/ContinuationIndenter.cpp?rev=263709&r1=263708&r2=263709&view=diff
>>>
>>> ==============================================================================
>>> --- cfe/trunk/lib/Format/ContinuationIndenter.cpp (original)
>>> +++ cfe/trunk/lib/Format/ContinuationIndenter.cpp Thu Mar 17 07:00:22
>>> 2016
>>> @@ -356,7 +356,17 @@ void ContinuationIndenter::addTokenOnCur
>>> Previous.isOneOf(tok::l_paren, TT_TemplateOpener, tok::l_square)
>>> &&
>>> State.Column > getNewLineColumn(State) &&
>>> (!Previous.Previous ||
>>> - !Previous.Previous->isOneOf(tok::kw_for, tok::kw_while,
>>> tok::kw_switch)))
>>> + !Previous.Previous->isOneOf(tok::kw_for, tok::kw_while,
>>> + tok::kw_switch)) &&
>>> + // Don't do this for simple (no expressions) one-argument
>>> function calls
>>> + // as that feels like needlessly wasting whitespace, e.g.:
>>> + //
>>> + // caaaaaaaaaaaall(
>>> + // caaaaaaaaaaaall(
>>> + // caaaaaaaaaaaall(
>>> + // caaaaaaaaaaaaaaaaaaaaaaall(aaaaaaaaaaaaaa,
>>> aaaaaaaaa))));
>>> + Current.FakeLParens.size() > 0 &&
>>> + Current.FakeLParens.back() > prec::Unknown)
>>> State.Stack.back().NoLineBreak = true;
>>>
>>> if (Style.AlignAfterOpenBracket != FormatStyle::BAS_DontAlign &&
>>>
>>> Modified: cfe/trunk/unittests/Format/FormatTest.cpp
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Format/FormatTest.cpp?rev=263709&r1=263708&r2=263709&view=diff
>>>
>>> ==============================================================================
>>> --- cfe/trunk/unittests/Format/FormatTest.cpp (original)
>>> +++ cfe/trunk/unittests/Format/FormatTest.cpp Thu Mar 17 07:00:22 2016
>>> @@ -4461,12 +4461,31 @@ TEST_F(FormatTest, AlignsAfterOpenBracke
>>> " aaaaaaaaaaa aaaaaaaaa,\n"
>>> " aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa);",
>>> Style);
>>> - verifyFormat("SomeLongVariableName->someFunction(\n"
>>> - " foooooooo(\n"
>>> - " aaaaaaaaaaaaaaa,\n"
>>> - " aaaaaaaaaaaaaaaaaaaaa,\n"
>>> - " aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa));",
>>> + verifyFormat("SomeLongVariableName->someFunction(foooooooo(\n"
>>> + " aaaaaaaaaaaaaaa,\n"
>>> + " aaaaaaaaaaaaaaaaaaaaa,\n"
>>> + " aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa));",
>>> Style);
>>> + verifyFormat(
>>> + "aaaaaaaaaaaaaaaaaaaaaaaa(aaaaaaaaaaaaaaaaaaaaa(\n"
>>> + " aaaaaaaaaaaaaaaaaaaa(aaaaaaaaaaaaaaaaa,
>>> aaaaaaaaaaaaaaaa)));",
>>> + Style);
>>> + verifyFormat(
>>> + "aaaaaaaaaaaaaaaaaaaaaaaa(aaaaaaaaaa.aaaaaaaaaa(\n"
>>> + " aaaaaaaaaaaaaaaaaaaa(aaaaaaaaaaaaaaaaa,
>>> aaaaaaaaaaaaaaaa)));",
>>> + Style);
>>> + verifyFormat(
>>> + "aaaaaaaaaaaaaaaaaaaaaaaa(\n"
>>> + " aaaaaaaaaaaaaaaaaaaaa(\n"
>>> + " aaaaaaaaaaaaaaaaaaaa(aaaaaaaaaaaaaaaaa,
>>> aaaaaaaaaaaaaaaa)),\n"
>>> + " aaaaaaaaaaaaaaaa);",
>>> + Style);
>>> + verifyFormat(
>>> + "aaaaaaaaaaaaaaaaaaaaaaaa(\n"
>>> + " aaaaaaaaaaaaaaaaaaaaa(\n"
>>> + " aaaaaaaaaaaaaaaaaaaa(aaaaaaaaaaaaaaaaa,
>>> aaaaaaaaaaaaaaaa)) &&\n"
>>> + " aaaaaaaaaaaaaaaa);",
>>> + Style);
>>> }
>>>
>>> TEST_F(FormatTest, ParenthesesAndOperandAlignment) {
>>>
>>>
>>> _______________________________________________
>>> cfe-commits mailing list
>>> cfe-commits at lists.llvm.org
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>>
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20160327/2afd9a4f/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: .clang-format
Type: application/octet-stream
Size: 2492 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20160327/2afd9a4f/attachment-0001.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: newTest.cpp
Type: text/x-c++src
Size: 6338 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20160327/2afd9a4f/attachment-0001.cpp>
More information about the cfe-commits
mailing list