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

[Wang, Lei]

On 9/1/2022 7:55 PM, 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'm not an Emacs guy, but it looks good to me.

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

That's necessary indeed.

>>
>> BR,
>> Lei
> 
>