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

[Alex Bennée]

"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.

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

>
> BR,
> Lei


-- 
Alex Bennée