Pekka Paalanen <[email protected]> writes: > On Thu, 14 Mar 2019 16:57:08 +0530 > Harish Krupo <[email protected]> wrote: > >> Harish Krupo <[email protected]> writes: >> >> > Hi Pekka, >> > >> > Pekka Paalanen <[email protected]> writes: >> > >> >> On Thu, 14 Mar 2019 16:01:08 +0530 >> >> Harish Krupo <[email protected]> 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 >> _______________________________________________ wayland-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/wayland-devel
