Hi Jean-Philippe, I have also been thinking some more about the naming and I think it is not entirely inaccurate. It is a setting of the option AlignAfterOpenBracket and setting that to "AlwaysBreak" means "always break instead of aligning". And it kind of makes sense that one-parameter functions are an exception as they never need aligning. Not saying it is perfect, though .. ;-).
Cheers, Daniel On Mon, Mar 28, 2016 at 2:27 PM, Jean-philippe Dufraigne < j.dufrai...@gmail.com> wrote: > Hi Daniel, > > Thanks, I understand why your users would have requested that and why you > need to provide this improvement for them. > I will survey the other people writing code with me on whether they are > happy with this improvement considering both the readability of the > statements and the relatively small impact in our day to day > reading/writing. > I'll get back to you, maybe I was wasting time on a non issue.. > > Cheers, > Jean-Philippe. > > > 2016-03-28 9:51 GMT+01:00 Daniel Jasper <djas...@google.com>: > >> 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.dufrai...@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 <djas...@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.dufrai...@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@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@lists.llvm.org >>>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits >>>>>> >>>>> >>>>> >>>> >>> >> >
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits