kicad-developers team mailing list archive
Mailing list archive
Re: Pcbnew display origin transforms for v6
On 06/05/2019 21:46, Wayne Stambaugh wrote:
On 5/6/19 4:09 PM, John Beard wrote:
On 06/05/2019 17:51, Reece R. Pollack wrote:
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_AUX:
- origin = m_PcbBaseFrame->GetAuxOrigin().y;
- case ORIGIN_REFERENCE_GRID:
- origin = m_PcbBaseFrame->GetGridOrigin().y;
+ case ORIGIN_REFERENCE_AUX: origin =
+ case ORIGIN_REFERENCE_GRID: origin =
+ 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:
It's the only line-condensing option we have set:
AllowShortCaseLabelsOnASingleLine: true # the only true here
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.