Re: Using clang-format

[Marco Barisione via Gdb-patches <gdb-patches@sourceware.org>]

On 11 May 2021, at 03:57, Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> wrote:
> 
> On 2021-05-10 10:55 p.m., Simon Marchi via Gdb-patches wrote:
>> On 2021-05-08 12:00 p.m., Tom Tromey wrote:
>>>>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:
>>> 
>>> Simon> A few people I talked to about this and I have good experience
>>> Simon> with the tool black to auto-format Python code.
>>> 
>>> Inspired by this, I took another look at clang-format.  If we could use
>>> this, I think it would be a decent productivity improvement, because it
>>> would mean contributors would no longer need to worry about the minutiae
>>> of the GNU style.  We could also stop reviewing this.
>> 
>> I would love this.

As an occasional contributor I’d love this.
Getting the correct style right is complicated if you spend most of your
time developing other projects with other coding styles.
It’s even worse for cases that are not trivial as GDB’s codebase is
occasionally inconsistent so there’s nowhere where to take inspiration
from.

>> I noticed this:
>> 
>>  btrace_block (CORE_ADDR begin, CORE_ADDR end) : begin (begin), end (end)
>>  {
>>    /* Nothing.  */
>>  }
>> 
>> The initializer list is put on the same line as the constructor's
>> prototype.  If there's a way to tell it that we don't want that, it
>> would be closer to our style.

You can use BreakConstructorInitializers.

>>> Another possible problem is that, unlike with 'black', ensuring that
>>> everyone has the same version is a pain.
>> 
>> Indeed.  It's perhaps easy for me to say, because I get to choose what
>> Linux distro and version I work on (so I opt for something recent), but
>> I would still lean towards just following whatever the current latest
>> stable version is.  There might be new options in the latest stable
>> version we want to use, it would be nice not to have to wait years
>> before we can use them.  And I suppose there's a not too painful way to
>> get it for all the major distros out there (and for Windows and macOS,
>> there are binary releases).  And you can always run it in Docker or
>> something.
>> 
>> But we would probably encourage people to use git-clang-format, which
>> only reformats the lines you touched.  So, it wouldn't be a big problem
>> for people to use different versions, as it wouldn't create spurious
>> changes elsewhere in the file.  The differences in formatting would be
>> in lines you touch anyway.
>> 
>> Speaking of git-clang-format: from my experience, it is not super cool
>> when the code base isn't already formatted with the target formatting
>> style.  It causes discrepancies in style, because you have a mix of new
>> style (whatever lines were touched) and old style (whatever lines were
>> not touched) which can be annoying and distracting.  Or, it causes
>> slightly bigger diffs than you would get otherwise.  For example, I
>> ran git-clang-format on this commit here:
>> 
>>    https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=10e578d7e00d
>> 
>> ... to see what would have been the resulting patch had the author used
>> git-clang-format.  This would have been the patch:
>> 
>>    https://pastebin.com/pxrZtnDU
>> 
>> If you check the changes in mi/mi-cmd-break.c, you can see that
>> git-clang-format re-formatted the whole enum and array initializer.
>> Which is nice in itself, the resulting formatting is nice.  But, the
>> patch would be a bit harder to read.
>> 
>> The other option is to start by doing a big re-format and then tell
>> people to use git-clang-format.  This way, there might be some small
>> incoherences here and there due to different versions, but I think
>> they'll be insignificant.
>> 
>> With the Python code, we did a massive re-format.  I don't think it did
>> / will cause a big inconvenience, because there aren't many patches to
>> the Python code.  But for the C++ code, it might be different, as there
>> are more patches in everyone's pipeline that touch that code.

Personally, I’d be a bit worried about dealing with difficult conflicts
manually, but there are solutions for that:

1. Rebase your branch to the point just before reformatting:
     $ git rebase <commit before clang-format commit ID>

2. Rebase on top of the format changes but preferring your local
   changes:

     $ git rebase --strategy=recursive \
           --strategy-option=theirs <clang-format commit ID>

3. Finally reformat all of your code you just committed and amend
   your previous commits.


At work we decided not to reformat the whole C/C++ code base (while
we did for Python).  Instead, we use some git hooks I wrote which
reformat only what you are committing.  See
https://github.com/barisione/clang-format-hooks.

While I wrote these scripts and would be happy for others to use
them, I think that (in GDB’s case) it would be easier to just
reformat everything and deal with the short-term pain. 


-- 
Marco Barisione