kicad-developers team mailing list archive

[Wayne Stambaugh <stambaughw@xxxxxxxxx>]

On 3/16/2017 6:03 AM, John Beard wrote:
> Hi Chris,
> 
> This is definitely loads better than uncrustify. I think it would be
> great if we can get a really solid config here that is officially
> accepted as "the" reference format, especially as it integrates well
> with editors (unlike uncrustify). I find it very hard to spot all
> formatting mistakes I make, especially as KiCad is already unique in
> format but not always consistent: when everything kind of stands out,
> nothing does, if that makes sense.
> 
> The official KiCad style guide is very short on actual details about
> formatting. Specifically, indentation of continued lines is something
> I never know how to do:
> 
> sometype somevar = somefunction( foobarbarfooquuxbar,
>         foobarbarfooquuxbar().GetBar() );

This was one of those formatting issues we never could agree on and
that's why you see the various styles.  I personally prefer argument
alignment but I can live with indented.

> 
> or (as hinted in the Strings section of the style guide):
> 
> sometype somevar = somefunction( foobarbarfooquuxbar,
> 
> foobarbarfooquuxbar().GetBar() );
> 
> Also, constructor parameter/init lists:
> 
> FOOBAR_CLASS::FOOBAR_CLASS( foobarbarfooquuxbar,
>         foobarbarfooquuxbar ) : // what indent here, space before colon?
>                 BASE_CLASS( 0 ), // and what indent here?
>                 m_member( 0 )
> {}
> 
> As for items in the _format-clang file that Chris submitted:
> 
> * I personally quite like the BinPackArguments: false style (each
> argument on new line) but that doesn't seem to be the normal style,
> and it does take quite a bit of space.

I prefer not to put every argument on it's own line.  It takes up too
much vertical space.  The only exception would be when the argument list
exceeds the 100 character maximum line length.

> * ContinuationIndentWidth: 4 seems a bit shallow (assuming this is how
> we want to indent, see above).

Assuming we would use indent continuation than 8 would be a preferable
indent.

> 
> It also doesn't seem to format things like this in "table" style for
> the second parameter:
> 
>     { KIGFX::OPENGL_ANTIALIASING_MODE::NONE, 0 }, // Default
>     { KIGFX::OPENGL_ANTIALIASING_MODE::SUBSAMPLE_HIGH, 1 },

I prefer the table row on a single line assuming the 100 character line
length is not exceeded.  I always prefer to never allow my editor to
wrap lines that exceed 100 characters.

> 
> Can we live without this for the sake of having a proper way to check
> formatting? You can still add "// clang-format off" if it's critical.

Until we make any formal changes, I will stand by my usual advice of
trying to match existing code as closely as possible.  If you are
creating a new file, than I'm comfortable leaving the undefined code
formatting up to the author.  Please do not take my comments as a sign
that I want to open this discussion.  I do not have the time or
motivation right now.  After the next stable release is done, maybe I'll
find the time and motivation to reopen this.

Cheers,

Wayne

> 
> Cheers,
> 
> John
> 
> 
> On Mon, Mar 13, 2017 at 9:48 PM, Wayne Stambaugh <stambaughw@xxxxxxxxx> wrote:
>> I'm OK with this.  If clang handles indenting the _() macro properly,
>> that is definitely an improvement over uncrustify.
>>
>> On 3/13/2017 9:42 AM, Chris Pavlina wrote:
>>> Hi,
>>>
>>> Currently, our preferred code formatting tool is Uncrustify. Uncrustify
>>> is bad. It does things like this:
>>>
>>> wxString m_choiceAntialiasingChoices[] =
>>>         {
>>>             _( "No Antialiasing" ),
>>>             _( "Subpixel Antialiasing (High Quality)" ),      _(
>>>                     "Subpixel Antialiasing (Ultra Quality)" ),_( "Supersampling (2x)" ), _(
>>>                     "Supersampling (4x)" )
>>>         };
>>>
>>> Yes, that's an actual Uncrustify output with our configuration, posted
>>> on IRC this morning.
>>>
>>> Per metacollin's suggestion I looked into clang-format, which does a
>>> MUCH nicer job because it uses clang's parser to actually understand the
>>> code it's formatting. I wrote up a clang-format config following the
>>> KiCad coding style. Here is the output of clang-format on the same code:
>>>
>>> wxString m_choiceAntialiasingChoices[] = { _( "No Antialiasing" ),
>>>     _( "Subpixel Antialiasing (High Quality)" ),
>>>     _( "Subpixel Antialiasing (Ultra Quality)" ),
>>>     _( "Supersampling (2x)" ),
>>>     _( "Supersampling (4x)" ) };
>>>
>>> It's not perfect, but it's way better than Uncrustify.
>>>
>>> The attached patch adds a _clang-format configuration file to the root
>>> of the repository.
>>>
>>>
>>>
>>> _______________________________________________
>>> Mailing list: https://launchpad.net/~kicad-developers
>>> Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
>>> Unsubscribe : https://launchpad.net/~kicad-developers
>>> More help   : https://help.launchpad.net/ListHelp
>>>
>>
>> _______________________________________________
>> Mailing list: https://launchpad.net/~kicad-developers
>> Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
>> Unsubscribe : https://launchpad.net/~kicad-developers
>> More help   : https://help.launchpad.net/ListHelp