kicad-developers team mailing list archive

[Wayne Stambaugh <stambaughw@xxxxxxxxx>]

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