r263709 - clang-format: Slightly weaken AlignAfterOpenBracket=AlwaysBreak.
Daniel Jasper via cfe-commits
cfe-commits at lists.llvm.org
Mon Mar 28 01:51:16 PDT 2016
Hi Jean-Philippe,
no, you have not convinced me with those examples. There is not a single
one, I find more readable. Many of them I find equally unreadable and would
write the code differently. But I would never have chose AlwaysBreak for my
own code in the first place and I rarely work in a codebase that uses it.
When I do, it usually bugs me. So, that (my opinion) is completely beside
the point.
There is one fundamental half-way objective argument you can make here (and
in many other cases): Being more compact generally makes it easier to read
larger chunks of code as it is more likely for a whole function/loop/... to
fit on a single page. Using more linebreaks to make the syntactic structure
clearer and more uniform can make individual statements more readable. My
argument is that in this case, often many lines are saved with very little
to no impact on local readability. You disagree and that's fine, I am not
trying to convince you. People a different here.
If you need the uniformity so that many subsequent statements are formatted
equally (e.g. in your "connRegist.Add( ..." case), you are likely doing it
wrong. Try harder not to repeat yourself. The problem here is that such
uniformly appearing sequence can hide very subtle (copy&paste) errors.
However, even all of this is beside the point.
You found changes in ~10% of your files. I'd bet that less than 10% of a
file was changed on average. So, this influences <1% of the code. That is
reasonably rare and you won't stumble over this frequently in practice.
Yes, of course you see hundreds of examples when you reformat a
thousand-file codebase and I understand why that bugs you.
I definitely need the new behavior as it was requested frequently by my
internal users. This was the most frequent bug report I have gotten from
people using AlwaysBreak. I suggest that you also speak to the other
authors of the codebase you are working on to make sure that you are
actually getting a feel for what most people want here. And also include
cases where it is significantly better IMO:
call(call(call(call(call(call(call(call(call(call(call(
parameterThatGoesUpToTheColumnLimit)))))))))));
vs.
call(
call(
call(
call(
...
parameterThatGoesUpToTheColumnLimit))))))))))); //
probably even a column limit violation
Again, I am not against adding an additional option. And if we do introduce
a new option, we should probably make that one strict and not have the
other exceptions.
Cheers,
Daniel
On Sun, Mar 27, 2016 at 8:16 PM, Jean-philippe Dufraigne <
j.dufraigne at gmail.com> wrote:
> 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/20160328/20986ec9/attachment-0001.html>
More information about the cfe-commits
mailing list