Clang format config for Weston

[Pekka Paalanen]

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.

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

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


Thanks,
pq

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20190314/93bedc7e/attachment.sig>