Clang format config for Weston

[Harish Krupo]

Pekka Paalanen <ppaalanen at gmail.com> writes:

> On Thu, 14 Mar 2019 16:57:08 +0530
> Harish Krupo <harish.krupo.kps at intel.com> wrote:
>
>> Harish Krupo <harish.krupo.kps at intel.com> writes:
>> 
>> > Hi Pekka,
>> >
>> > Pekka Paalanen <ppaalanen at gmail.com> writes:
>> >  
>> >> On Thu, 14 Mar 2019 16:01:08 +0530
>> >> Harish Krupo <harish.krupo.kps at intel.com> wrote:
>> >>  
>> >>> Hi,
>> >>> 
>> >>> I have written a clang format file based on this example [1], to
>> >>> match the coding style here [2]. Does the below config look okay or
>> >>> should something be changed?
>> >>> If people are interested, I can open a MR for this. This could also be
>> >>> used in the CI to warn/abort if a patch isn't according to the coding
>> >>> style.
>> >>> 
>> >>> The config:
>> >>> BasedOnStyle: LLVM
>> >>> IndentWidth: 8
>> >>> TabWidth: 8
>> >>> UseTab: Always
>> >>> BreakBeforeBraces: Linux
>> >>> AllowShortIfStatementsOnASingleLine: false
>> >>> IndentCaseLabels: false
>> >>> AlwaysBreakAfterReturnType: TopLevel
>> >>> 
>> >>> [1] https://clang.llvm.org/docs/ClangFormatStyleOptions.html#examples
>> >>> [2] https://gitlab.freedesktop.org/wayland/weston/blob/master/CONTRIBUTING.md#coding-style  
>> >>
>> >> Hi Harish,
>> >>
>> >> I guess that depends on how different that is from the existing code
>> >> base style. Seeing the warning list generated for the existing code
>> >> would tell a lot, I doubt it would be empty.
>> >>  
>> >
>> > Okay, I see what you mean. Running clang format on
>> > libweston/gl-renderer.c does generate a few false positives. Some of
>> > them could be corrected by setting other config options.
>> >  
>> >> Is the LLVM style something guaranteed to not change?  
>> >
>> > I am not sure about this :(
>> >  
>> >>
>> >> Using it in CI might be an attractive idea, but I wonder if it would
>> >> result in many false warnings. Some aspects of coding style are always
>> >> somewhat vague and need to adapt to the code at hand to look nice to a
>> >> human rather than follow some rigorous rules. If it leaves such things
>> >> as is and checks only those that should follow rigorous rules, that
>> >> would be nice.
>> >>  
>> >
>> > All the config options that Clang-format uses to format code is given in
>> > [1]. It leaves the rest to the user. We would have to define all (at
>> > least most) of those options for weston if we want to use this with the
>> > CI. Consequence of doing this would make the BasedOnStyle: LLVM option
>> > unneeded.
>> >  
>> 
>> Another thing to note is that clang format provides a tool called
>> clang-format-diff which runs the checker only for diffs. This way we
>> don't have to change the existing code. Only new changes can be checked
>> this way. It can be run as follows:
>> `git diff | clang-format-diff.py -p1 -style=file`
>> or
>> `git show <patch> | clang-format-diff.py -p1 -style=file`
>
> Hi,
>
> yes, I was already assuming there is a way to run it only on new
> patches.
>
> The test with existing code however is a good indication on how far the
> clang format definition would go: if the places it highlighted were
> fixed, would the readability get better or worse.
>
> We should go over each pattern Clang highlights and see which way is
> better. Once it gets into CI, people will be hell-bent to fix
> everything it points out, even if it's just a warning rather than
> failure.
>

Understood. The behaviour for each of the pattern that it checks is
documented. We would need to decide on what action it should to take on
those patterns for weston.

> Style checker as part of CI would be awesome, we just have to make sure
> it is more help than annoyance.
>

Agreed.

> FYI, one thing I would never want to see in a patch is adding
> '/* clang-format off */' kind of directives into code.
>

/* clang-format off */ directive is supposed to be used sparingly. If
the whole code is commented out that way then I believe the reviewer
should point that out. We could also add a check in the CI to
ensure that the /* clang-format off */ and /* clang-format on */ pair
don't span more than a few lines at a time.

Thank you
Regards
Harish Krupo

>
> Thanks,
> pq
>
>> 
>> > Thank you
>> > Regards
>> > Harish Krupo
>> >
>> > [1] https://clang.llvm.org/docs/ClangFormatStyleOptions.html  
>>