Re: [Qemu-devel] [RFC PATCH] Add qemu .clang-format

[Daniel P . Berrangé]

On Thu, Sep 01, 2022 at 12:55:36PM +0100, Alex Bennée wrote:
> 
> "Wang, Lei" <lei4.wang@intel.com> writes:
> 
> > On 9/1/2022 4:12 PM, Daniel P. Berrangé wrote:
> >> On Thu, Sep 01, 2022 at 09:08:33AM +0800, Wang, Lei wrote:
> >>> On 8/31/2022 6:39 PM, Daniel P. Berrangé wrote:
> >>>> On Wed, Aug 31, 2022 at 05:18:34PM +0800, Wang, Lei wrote:
> >>>>>
> >>>>>
> >>>>> On 8/31/2022 4:49 PM, Daniel P. Berrangé wrote:
> >>>>>> On Wed, Aug 31, 2022 at 02:23:51PM +0800, Wang, Lei wrote:
> >>>>>>>
> >>>>>>> On 10/2/2015 1:30 AM, marcandre.lureau@redhat.com wrote:
> >>>>>>>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >>>>>>>>
> >>>>>>>> clang-format is awesome to reflow your code according to qemu coding
> >>>>>>>> style in an editor (in the region you modify).
> >>>>>>>>
> >>>>>>>> (note: clang-tidy should be able to add missing braces around
> >>>>>>>> statements, but I haven't tried it, it's quite recent)
> >>>>>>>>
> >>>>>>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> >>>>>>>> ---
> >>>>>>>>     .clang-format | 6 ++++++
> >>>>>>>>     1 file changed, 6 insertions(+)
> >>>>>>>>     create mode 100644 .clang-format
> >>>>>>>>
> >>>>>>>> diff --git a/.clang-format b/.clang-format
> >>>>>>>> new file mode 100644
> >>>>>>>> index 0000000..6422547
> >>>>>>>> --- /dev/null
> >>>>>>>> +++ b/.clang-format
> >>>>>>>> @@ -0,0 +1,6 @@
> >>>>>>>> +BasedOnStyle: LLVM
> >>>>>>>> +IndentWidth: 4
> >>>>>>>> +UseTab: Never
> >>>>>>>> +BreakBeforeBraces: Linux
> >>>>>>>> +AllowShortIfStatementsOnASingleLine: false
> >>>>>>>> +IndentCaseLabels: false
> >>>>>>>
> >>>>>>> Hi, any progress on this? I also found a gist on GitHub which can be a
> >>>>>>> reference: 
> >>>>>>> https://gist.github.com/elmarco/aa5e0b23567f46fb7f0e73cde586a0c1
> >>>>>>
> >>>>>> clang-format is a great tool and I'd highly recommend its use on
> >>>>>> any newly started projects, and even retrospectively on existing
> >>>>>> projects which are small scale. Adding it to large existing projects
> >>>>>> is problematic though.
> >>>>>>
> >>>>>> None of the QEMU code complies with it today and indeed there is
> >>>>>> quite a bit of style variance across different parts of QEMU. If
> >>>>>> we add this config file, and someone makes a 1 line change in a
> >>>>>> file, clang-format will reformat the entire file contents.
> >>>>>>
> >>>>>> The only practical way to introduce use of clang-format would be
> >>>>>> to do a bulk reformat of the entire codebase. That is something
> >>>>>> that is quite disruptive to both people with patches they're
> >>>>>> working on but not submitted yet, as well as people wanting to
> >>>>>> cherry-pick new commits back to old code branches.
> >>>>>>
> >>>>>> With regards,
> >>>>>> Daniel
> >>>>>
> >>>>> I think the benefits of introducing clang-format mainly for its ability 
> >>>>> to
> >>>>> format a code range, which means for any future contributions, we could
> >>>>> encourage a range format before the patch is generated. This can 
> >>>>> extensively
> >>>>> simplify my workflow, especially because I use the Neovim + LSP 
> >>>>> combination,
> >>>>> which supports a built-in function "lua vim.lsp.buf.range_formatting()".
> >>>>
> >>>> IMHO partial format conversions are even worse than full conversions,
> >>>> because they would make code inconsistent within the scope of a file.
> >>>
> >>> So you mean when we're adding new code in an old file, the coding style
> >>> should also be the old one? That sounds a bit unreasonable. I thought we 
> >>> are
> >>> shifting the coding style in an on-demand way, so we can finally achieve 
> >>> to
> >>> the new style mildly, if each time we're using the old coding style, that
> >>> could be impossible.
> >> 
> >> From my POV as a maintainer, the best situation would be consistency across
> >> the entire codebase. Since we likely won't get that though, then next best
> >> is consistency across the subsystem directory, and next best is consistency
> >> across the whole file.  Mixing code styles within a file is the worst IMHO.
> >> 
> >>>
> >>>>> I have no interest in reformatting the existing code and also think 
> >>>>> using it
> >>>>> to reformat an entire file shouldn't be encouraged, but, we can leverage
> >>>>> this tool to give future contributions a better experience. It's also
> >>>>> important to note that the kernel already has a ".clang-format" file, 
> >>>>> so I
> >>>>> think we can give it a try:)
> >>>>
> >>>> The mere action of introducing a .clang-format file in the root of the
> >>>> repository will cause some contributors' editors to automatically
> >>>> reformat files every time they are saved. IOW even if you don't want
> >>>> intend to do reformatting, that will be a net result.
> >>>
> >>> I think that depends on developer's configuration, as far as I know, 
> >>> format
> >>> on save is a feature which can be easily disabled on most of the IDE's, 
> >>> such
> >>> as VSCode.
> >> 
> >> You could disable it, but it requires each developer to know that we're
> >> shipping a clang-format that should not in fact be used to reformat
> >> code, which is rather counterintuitive. 
> >> 
> >> With regards,
> >> Daniel
> >
> > OK, your POV makes sense too. I think we can do a tradeoff, for an example, 
> > we
> > can add an officially suggested ".clang-format" file in the documentation, 
> > so it
> > won't confuse the developers who have no interest in the clang stuffs, and 
> > it
> > will also be more convenient for the developers who don't want to check the
> > coding style manually each time before they're submitting a patch.
> 
> For most editors we already have a .editorconfig but it looks like there
> is no integration for clang-format there. We could put a file in
> contrib/style/ for an explicit call:
> 
>   clang-format -style=contrib/style/clang.format
> 
> I suspect we should move the .dir-locals there to given Emacs users
> should be able to use the .editorconfig and it reduces duplication.

I'd be against that on the basis that EditorConfig support is an
add-on for emacs which we can't be sure our contributors will have
installed. The .dir-locals ensures we get sensible behaviour from
all emacs users by default.

> And of course mention the location of these style linters in
> docs/devel/style.rst

Note clang-format is not a style linter - it is a bulk code
reformatter that does waaaaaaay more than the .editorconfig
or .dir-locals does - our code is largely compliant with
the .editorconfig/.dir-locals rules, so it is good that we
ensure continued compliance by default.  clang-format is a
completely different situation as essentially none of our
code will be compliant today.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|