John, On 5/6/2019 5:11 PM, John Beard wrote: > > > On 06/05/2019 21:46, Wayne Stambaugh wrote: >> John, >> >> On 5/6/19 4:09 PM, John Beard wrote: >>> On 06/05/2019 17:51, Reece R. Pollack wrote: >>>> John, >>>> >>>> I've already jumped to clang-format 6.0, which is one of the optional >>>> installs for Mint 18. That works, once you get all the symlinks fixed, >>> >>> Good to know, thanks for the update. >>> >>>> except it keeps wanting to reformat my switch statements like this, >>>> which is contrary to the KiCad coding standards: >>>> >>>> @@ -148,15 +130,9 @@ int PCB_ORIGIN_TRANSFORM_Y_ABS::FromDisplay( int >>>> aValue ) const >>>> case ORIGIN_REFERENCE_PAGE: >>>> // No-op >>>> break; >>>> - case ORIGIN_REFERENCE_AUX: >>>> - origin = m_PcbBaseFrame->GetAuxOrigin().y; >>>> - break; >>>> - case ORIGIN_REFERENCE_GRID: >>>> - origin = m_PcbBaseFrame->GetGridOrigin().y; >>>> - break; >>>> - default: >>>> - wxASSERT(false); >>>> - break; >>>> + case ORIGIN_REFERENCE_AUX: origin = >>>> m_PcbBaseFrame->GetAuxOrigin().y; break; >>>> + case ORIGIN_REFERENCE_GRID: origin = >>>> m_PcbBaseFrame->GetGridOrigin().y; break; >>>> + default: wxASSERT( false ); break; >>>> } >>>> >>>> // Invert the direction if needed >>> >>> This is a weird style that I don't personally like, and IMO goes against >>> the style implied by our "spacious" bracing and newline-before-if >>> policies. But there are quite some uses of it. It's enforced by this >>> line: >>> >>> AllowShortCaseLabelsOnASingleLine: true >>> >>> It's the only line-condensing option we have set: >>> >>> AllowShortBlocksOnASingleLine: false >>> AllowShortCaseLabelsOnASingleLine: true # the only true here >>> AllowShortFunctionsOnASingleLine: false >>> AllowShortIfStatementsOnASingleLine: false >>> AllowShortLoopsOnASingleLine: false >>> >>> If the formatter is proposing a change that goes against the existing >>> style in the code area in question, I do not think it's controversial to >>> ignore its suggestion. >>> >>> @Wayne, what do you think: is this enforcement representative of the >>> right style? Should we change AllowShortCaseLabelsOnASingleLine to >>> false? (+1 from me). >> >> I would rather not have clang-format force this change. I am OK if the >> developer prefers this for short case labels but if they prefer a >> separate line that's fine. I prefer everything on more than one line >> but if I'm changing code that is already formatted on a single line, I >> don't change it. We didn't specify this in the coding policy so it's a >> don't care but I would ask you to change it if you your case labels were >> complex and you had them on a single line. > > Fair enough - gratuitous formatting changes in the same commit as > functional changes are a sin anyway! > > Remember, (git-)clang-format will only change what you change, it > shouldn't forcibly change formatting in general unless you tell it to. > Users still need to ensure it looks sane and only accept good changes. > > Having AllowShortCaseLabelsOnASingleLine = true means that clang-format > will *enforce* the all-on-a-line policy when it can, but it will not > enforce column alignment (and will actually collapse manual column > alignment). So basically, the formatter is unlikely to ever give a good > result for a short-case switch as it stands. > > I still suggest to change it to false be default and allow developers to > manually align when they want (and then override the formatter). Then at > least the default behaviour is a valid formatting choice. > > Cheers, > > John
That works for me. Thanks, Wayne _______________________________________________ Mailing list: https://launchpad.net/~kicad-developers Post to : kicad-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~kicad-developers More help : https://help.launchpad.net/ListHelp