kicad-developers team mailing list archive

[John Beard <john.j.beard@xxxxxxxxx>]

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