Proposal: format GDB Python files with black

Proposal: format GDB Python files with black

[Simon Marchi]

Hi all,

A few people I talked to about this and I have good experience with the
tool black to auto-format Python code.  It is simple to use, fast and
reliable (if it detects that it changed the meaning of the program by
mistake, internal-errors out).  I don't think I need to spell out the
advantage of using such a tool, but just in case: it removes all the
overhead of thinking about formatting when writing or reviewing code.

My suggestion would be to format our code with black all at once.  The
typical counter-argument to this is "it will be harder to use
git-blame".  I don't think this is true, because you need to be able to
skip over commits anyway, and it's trivial to skip over a commit when
git-blaming using an interactive tool.  But it's also possible to tell
git to ignore certain commits when git-blaming [2], so we can do that.

I think the output of black is quite readable.  When it does weird
stuff, it's generally because the lines / expressions are two long
anyway, and deserve to be split in multiple lines / expressions.  Here's
a branch that shows how it would look like:

  https://sourceware.org/git/?p=binutils-gdb.git;a=shortlog;h=refs/heads/users/simark/black

If the feedback is overall positive, I'll send a more format patch.

Simon

[1] https://github.com/psf/black
[2] https://github.com/psf/black#migrating-your-code-style-without-ruining-git-blame

Re: Proposal: format GDB Python files with black

[Andrew Burgess]

* Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> [2021-04-26 11:55:21 -0400]:

> Hi all,
> 
> A few people I talked to about this and I have good experience with the
> tool black to auto-format Python code.  It is simple to use, fast and
> reliable (if it detects that it changed the meaning of the program by
> mistake, internal-errors out).  I don't think I need to spell out the
> advantage of using such a tool, but just in case: it removes all the
> overhead of thinking about formatting when writing or reviewing code.
> 
> My suggestion would be to format our code with black all at once.  The
> typical counter-argument to this is "it will be harder to use
> git-blame".  I don't think this is true, because you need to be able to
> skip over commits anyway, and it's trivial to skip over a commit when
> git-blaming using an interactive tool.  But it's also possible to tell
> git to ignore certain commits when git-blaming [2], so we can do that.
> 
> I think the output of black is quite readable.  When it does weird
> stuff, it's generally because the lines / expressions are two long
> anyway, and deserve to be split in multiple lines / expressions.  Here's
> a branch that shows how it would look like:

In general, the idea of auto-formatting the code sounds great, but I
would like to understand more about how the process would work.

Right now, if I make a change I edit a file, git add, git commit, and
finally, git push.  At what point does the auto-formatting take place?
Do I need to manually run the tool after editing the file?  Is it
done, well, automatically at some point?  And if it is done
automatically, then at what point in the process does this happen, and
how does this relate to code review?

Thanks,
Andrew

Re: Proposal: format GDB Python files with black

[Simon Marchi]

On 2021-04-26 12:21 p.m., Andrew Burgess wrote:
> * Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> [2021-04-26 11:55:21 -0400]:
> 
>> Hi all,
>>
>> A few people I talked to about this and I have good experience with the
>> tool black to auto-format Python code.  It is simple to use, fast and
>> reliable (if it detects that it changed the meaning of the program by
>> mistake, internal-errors out).  I don't think I need to spell out the
>> advantage of using such a tool, but just in case: it removes all the
>> overhead of thinking about formatting when writing or reviewing code.
>>
>> My suggestion would be to format our code with black all at once.  The
>> typical counter-argument to this is "it will be harder to use
>> git-blame".  I don't think this is true, because you need to be able to
>> skip over commits anyway, and it's trivial to skip over a commit when
>> git-blaming using an interactive tool.  But it's also possible to tell
>> git to ignore certain commits when git-blaming [2], so we can do that.
>>
>> I think the output of black is quite readable.  When it does weird
>> stuff, it's generally because the lines / expressions are two long
>> anyway, and deserve to be split in multiple lines / expressions.  Here's
>> a branch that shows how it would look like:
> 
> In general, the idea of auto-formatting the code sounds great, but I
> would like to understand more about how the process would work.
> 
> Right now, if I make a change I edit a file, git add, git commit, and
> finally, git push.  At what point does the auto-formatting take place?
> Do I need to manually run the tool after editing the file?  Is it
> done, well, automatically at some point?  And if it is done
> automatically, then at what point in the process does this happen, and
> how does this relate to code review?

Those are good questions.

At the simplest level, the individual making changes is responsible for
the tool at any time they want, as long as it's before sending the patch
and / or pushing to master.  So it would be pretty much like today,
where we are responsible for formatting the source code correctly,
except that it would now be tool-assisted instead of done mentally.

How to run the tool is left to each individual:

1. By hand on the command line
2. From the editor [1]
3. Using a git-commit hook

If we want to be a bit more sophisticated, we could check-in a git
commit hook that runs black (if available) to check your formatting as
you are committing, if your commit includes changes to Python files.
That doesn't prevents formatting errors from slipping in the repo, but
it's already a good start (and formatting errors slipping in the repo
are not the end of the world).

We don't really have CI right now (especially pre-merge testing), but if
we had, checking the formatting of Python files could be a validation
step.

We do however, have scripts that run server side on push, so it would be
possible to run black server-side to reject commits that would introduce
formatting errors.

Simon

[1] https://black.readthedocs.io/en/stable/editor_integration.html

Re: Proposal: format GDB Python files with black

[Paul Koning]

> On Apr 26, 2021, at 11:55 AM, Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> wrote:
> 
> Hi all,
> 
> A few people I talked to about this and I have good experience with the
> tool black to auto-format Python code.  It is simple to use, fast and
> reliable (if it detects that it changed the meaning of the program by
> mistake, internal-errors out).  I don't think I need to spell out the
> advantage of using such a tool, but just in case: it removes all the
> overhead of thinking about formatting when writing or reviewing code.

In the case of Python, formatting is a core part of writing the code.  Apart from that, I don't see much need for this, what with Emacs editing mechanisms.

I looked at the sample you mentioned.  It's reasonable enough except for the bizarre way of formatting a long parenthesized condition in an if statement (near the start of FrameDecorator.py).  So while I can't see any reason to use this, it seems ok as a tool that others might use if they like to do so.

I would object to having this mandatory.

	paul

Re: Proposal: format GDB Python files with black

[Tom Tromey]

>>>>> "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 with the
Simon> tool black to auto-format Python code.

Yeah, +1 from me.  We use it internally here and it has been fine.  And,
it eliminates an entire review step, which is good.

Tom

Re: Proposal: format GDB Python files with black

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:

Andrew> Right now, if I make a change I edit a file, git add, git commit, and
Andrew> finally, git push.  At what point does the auto-formatting take place?
Andrew> Do I need to manually run the tool after editing the file?  Is it
Andrew> done, well, automatically at some point?  And if it is done
Andrew> automatically, then at what point in the process does this happen, and
Andrew> how does this relate to code review?

Internally, we do it via the Python 'pre-commit' program.  Then if I
"git commit" in the repository, the formatter is run.
It's easy to run manually as well though.

For gdb I think it would relate in the same way that testing is done:
you just indicate that you did it.

Tom

Re: Proposal: format GDB Python files with black

[Simon Marchi]

On 2021-04-26 1:34 p.m., Paul Koning wrote:> 
> 
>> On Apr 26, 2021, at 11:55 AM, Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> wrote:
>>
>> Hi all,
>>
>> A few people I talked to about this and I have good experience with the
>> tool black to auto-format Python code.  It is simple to use, fast and
>> reliable (if it detects that it changed the meaning of the program by
>> mistake, internal-errors out).  I don't think I need to spell out the
>> advantage of using such a tool, but just in case: it removes all the
>> overhead of thinking about formatting when writing or reviewing code.
> 
> In the case of Python, formatting is a core part of writing the code.  Apart from that, I don't see much need for this, what with Emacs editing mechanisms.
> 
> I looked at the sample you mentioned.  It's reasonable enough except for the bizarre way of formatting a long parenthesized condition in an if statement (near the start of FrameDecorator.py).  So while I can't see any reason to use this, it seems ok as a tool that others might use if they like to do so.
> 
> I would object to having this mandatory.

The thing is that it's difficult for some people to use an auto-format
tool if not everybody use it.  You would always get some spurious
formatting changes, so you would need to hand-edit the patch after that
to avoid adding those.  You don't really save time.  What would save me
(and everybody) some cycles is something we could just run mindlessly.

For clang-format, there is git-clang-format that formats just the lines
touched by a commit, perhaps there's something similar with black.  That
sounds like a good idea, but in my experience you just end up with a
weird mix of styles that is even worse, and it still ends up
reformatting more than what you really changed.

Simon

Re: Proposal: format GDB Python files with black

[Simon Marchi]

On 2021-04-26 1:44 p.m., Simon Marchi via Gdb-patches wrote:> On 2021-04-26 1:34 p.m., Paul Koning wrote:> 
>>
>>> On Apr 26, 2021, at 11:55 AM, Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> wrote:
>>>
>>> Hi all,
>>>
>>> A few people I talked to about this and I have good experience with the
>>> tool black to auto-format Python code.  It is simple to use, fast and
>>> reliable (if it detects that it changed the meaning of the program by
>>> mistake, internal-errors out).  I don't think I need to spell out the
>>> advantage of using such a tool, but just in case: it removes all the
>>> overhead of thinking about formatting when writing or reviewing code.
>>
>> In the case of Python, formatting is a core part of writing the code.  Apart from that, I don't see much need for this, what with Emacs editing mechanisms.
>>
>> I looked at the sample you mentioned.  It's reasonable enough except for the bizarre way of formatting a long parenthesized condition in an if statement (near the start of FrameDecorator.py).  So while I can't see any reason to use this, it seems ok as a tool that others might use if they like to do so.
>>
>> I would object to having this mandatory.
> 
> The thing is that it's difficult for some people to use an auto-format
> tool if not everybody use it.  You would always get some spurious
> formatting changes, so you would need to hand-edit the patch after that
> to avoid adding those.  You don't really save time.  What would save me
> (and everybody) some cycles is something we could just run mindlessly.
> 
> For clang-format, there is git-clang-format that formats just the lines
> touched by a commit, perhaps there's something similar with black.  That
> sounds like a good idea, but in my experience you just end up with a
> weird mix of styles that is even worse, and it still ends up
> reformatting more than what you really changed.

To be clear, I wouldn't really mind if some contributors didn't use it
(for the same reason that it's not the end of the world if formatting
errors are introduced).  If I detect that a file I want to modify is
not black-compliant, I'll just make a preparatory commit "Format foo.py
with black" and make my patch on top of that.  But I would appreciate if
we could do a first pass of formatting all the code, to have a good
baseline.

Simon

Re: Proposal: format GDB Python files with black

[Andrew Burgess]

* Simon Marchi <simon.marchi@polymtl.ca> [2021-04-26 13:25:37 -0400]:

> On 2021-04-26 12:21 p.m., Andrew Burgess wrote:
> > * Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> [2021-04-26 11:55:21 -0400]:
> > 
> >> Hi all,
> >>
> >> A few people I talked to about this and I have good experience with the
> >> tool black to auto-format Python code.  It is simple to use, fast and
> >> reliable (if it detects that it changed the meaning of the program by
> >> mistake, internal-errors out).  I don't think I need to spell out the
> >> advantage of using such a tool, but just in case: it removes all the
> >> overhead of thinking about formatting when writing or reviewing code.
> >>
> >> My suggestion would be to format our code with black all at once.  The
> >> typical counter-argument to this is "it will be harder to use
> >> git-blame".  I don't think this is true, because you need to be able to
> >> skip over commits anyway, and it's trivial to skip over a commit when
> >> git-blaming using an interactive tool.  But it's also possible to tell
> >> git to ignore certain commits when git-blaming [2], so we can do that.
> >>
> >> I think the output of black is quite readable.  When it does weird
> >> stuff, it's generally because the lines / expressions are two long
> >> anyway, and deserve to be split in multiple lines / expressions.  Here's
> >> a branch that shows how it would look like:
> > 
> > In general, the idea of auto-formatting the code sounds great, but I
> > would like to understand more about how the process would work.
> > 
> > Right now, if I make a change I edit a file, git add, git commit, and
> > finally, git push.  At what point does the auto-formatting take place?
> > Do I need to manually run the tool after editing the file?  Is it
> > done, well, automatically at some point?  And if it is done
> > automatically, then at what point in the process does this happen, and
> > how does this relate to code review?
> 
> Those are good questions.
> 
> At the simplest level, the individual making changes is responsible for
> the tool at any time they want, as long as it's before sending the patch
> and / or pushing to master.  So it would be pretty much like today,
> where we are responsible for formatting the source code correctly,
> except that it would now be tool-assisted instead of done mentally.
> 
> How to run the tool is left to each individual:
> 
> 1. By hand on the command line
> 2. From the editor [1]
> 3. Using a git-commit hook

So without going more sophisticated as you describe below we'd still
be relying on reviewers to either (with enough experience) "just know"
that the code is black formatted, or apply the patch, run black, and
see if the formatting changes.

Then of course, there's the questions about what happens when
different users are running different versions of black - assuming
different versions might format things slightly differently.

Another concern I'd have is that unrelated "formatting" changes might
creep in.

So, I edit a .py file and either don't run black (naughty), or use
version X of black.

Much later, I edit a different part of the same file, now either I run
black, or run version Y of black.  My original edit is reformatted
slightly differently.

My assumption is that this change (the second patch) should be
rejected at review, as the reformatting change (of the earlier code)
is unrelated to the work of the second patch.  But I suspect some folk
might find it frustrating, if on one had we say run black, but on the
other hand we say you need to exclude unrelated chunks.

I think, in the heterogeneous environments we all develop in,
situations like the above will crop up, so it would be important to
know what the expectations would be in such a case.

> 
> If we want to be a bit more sophisticated, we could check-in a git
> commit hook that runs black (if available) to check your formatting as
> you are committing, if your commit includes changes to Python files.
> That doesn't prevents formatting errors from slipping in the repo, but
> it's already a good start (and formatting errors slipping in the repo
> are not the end of the world).

Again, this sort of thing works great in a corporate environment where
you can guarantee that everyone is (or has no excuse not to be)
running the exact same version of every tool.

My concern would be that such a strategy would invite unintended
changes to creep in.

> 
> We don't really have CI right now (especially pre-merge testing), but if
> we had, checking the formatting of Python files could be a validation
> step.

Agreed.

> We do however, have scripts that run server side on push, so it would be
> possible to run black server-side to reject commits that would introduce
> formatting errors.

I guess this would solve a lot of the problems I'm concerned about.
Once we've "corrected" the formatting of all .py code then nobody
could merge bad code.

Thanks for all the info,

Andrew

Re: Proposal: format GDB Python files with black

[Simon Marchi]

On 2021-04-26 1:50 p.m., Andrew Burgess wrote:
> So without going more sophisticated as you describe below we'd still
> be relying on reviewers to either (with enough experience) "just know"
> that the code is black formatted, or apply the patch, run black, and
> see if the formatting changes.

The use of black would be documented somewhere (in the repo, in the
wiki, or both).  Nevertheless, I don't expect all contributors to know
about it and do it.

I also don't expect reviewers to be able to spot code that isn't
black-compliant.  If you think of checking it or asking about it in a
review, great.  Otherwise, I don't think we should be too afraid of
errors slipping in, since they're trivial to fix.

But to be frank, as a reviewer today, I don't really know what to check
in the Python code formatting.  Our wiki:

  https://sourceware.org/gdb/wiki/Internals%20GDB-Python-Coding-Standards

says that we follow PEP8.  I have no idea how to format PEP8 code, so
as a result I'm not really checking the formatting of Python code today.

> Then of course, there's the questions about what happens when
> different users are running different versions of black - assuming
> different versions might format things slightly differently.

Indeed, I should have mentioned this: we would define a version to use,
just like we do for autoconf/automake.

The goal of black is that the output won't change just for fun, but it
could change due to fixed bugs.

> Another concern I'd have is that unrelated "formatting" changes might
> creep in.
> 
> So, I edit a .py file and either don't run black (naughty), or use
> version X of black.
> 
> Much later, I edit a different part of the same file, now either I run
> black, or run version Y of black.  My original edit is reformatted
> slightly differently.
> 
> My assumption is that this change (the second patch) should be
> rejected at review, as the reformatting change (of the earlier code)
> is unrelated to the work of the second patch.  But I suspect some folk
> might find it frustrating, if on one had we say run black, but on the
> other hand we say you need to exclude unrelated chunks.

This is why I think it's easier to start from a clean state, by
formatting everything in one shot in the beginning.

If the issue you describe happens while I am writing a patch, I would
notice the spurious diff and push an obvious "Format foo.py with
black" commit and rebase my WIP patch on top.

If the issue you describe happens while I am revieweing a patch, I would
probably also push an obvious re-formatting commit (or ask the person to
do so, if they have push access).  The offending hunk will go away when
they rebase, which is necessary prior to pushing.

> I think, in the heterogeneous environments we all develop in,
> situations like the above will crop up, so it would be important to
> know what the expectations would be in such a case.

Indeed.  I hope I was able to show that these situations can be handled
easily and are not a showstopper.

>>
>> If we want to be a bit more sophisticated, we could check-in a git
>> commit hook that runs black (if available) to check your formatting as
>> you are committing, if your commit includes changes to Python files.
>> That doesn't prevents formatting errors from slipping in the repo, but
>> it's already a good start (and formatting errors slipping in the repo
>> are not the end of the world).
> 
> Again, this sort of thing works great in a corporate environment where
> you can guarantee that everyone is (or has no excuse not to be)
> running the exact same version of every tool.

Even in corporate environment, you can't assume that!

> My concern would be that such a strategy would invite unintended
> changes to creep in.
> 
>>
>> We don't really have CI right now (especially pre-merge testing), but if
>> we had, checking the formatting of Python files could be a validation
>> step.
> 
> Agreed.
> 
>> We do however, have scripts that run server side on push, so it would be
>> possible to run black server-side to reject commits that would introduce
>> formatting errors.
> 
> I guess this would solve a lot of the problems I'm concerned about.
> Once we've "corrected" the formatting of all .py code then nobody
> could merge bad code.

Thanks,

Simon

Re: Proposal: format GDB Python files with black

[Lancelot SIX]

Hi

I have been using black on a few project and it is overall an enjoyable
experience.  It is also quite nice to have for new contributors with
limited experience with a particular coding style in place.  It gets
this out of the way.

As this has been mentioned earlier in the discussion[1], black output
might change slightly between versions.  I sometimes had some continuous
integration build fail because the version on the development
environment and the one running `black --check` differ.  The differences
are minor but this is  worth keeping in mind if “false positive” are an
issue, for example if it rejects commits in server-side scripts as
mentioned in [2].  That being said, specifying which version to use
solves the problem easily.  Another option can be to use 'black' to
help format the code and 'pycodestyle'[3] to do the automated checks,
but I am not sure this is the way you want to go to, introducing more
and more tools.

I’d also like to point out that by default, 'black' will use 88 char
long lines[4], which is not compliant with the PEP8[5], which is, as you
pointed out[1], referenced in GDB's wiki[6].  Either the tool's
configuration or the standard can be adjusted, depending on the
maintainers preferences.

For what it is worth, I really think black is a great tool that can
simplify development workflow.

Lancelot.

[1] https://sourceware.org/pipermail/gdb-patches/2021-April/178209.html
[2] https://sourceware.org/pipermail/gdb-patches/2021-April/178200.html
[3] https://pypi.org/project/pycodestyle/
[4] https://black.readthedocs.io/en/stable/the_black_code_style.html#line-length
[5] https://www.python.org/dev/peps/pep-0008/#maximum-line-length
[6] https://sourceware.org/gdb/wiki/Internals%20GDB-Python-Coding-Standards

Re: Proposal: format GDB Python files with black

[Andrew Burgess]

* Simon Marchi <simon.marchi@polymtl.ca> [2021-04-26 14:08:24 -0400]:

> On 2021-04-26 1:50 p.m., Andrew Burgess wrote:
> > So without going more sophisticated as you describe below we'd still
> > be relying on reviewers to either (with enough experience) "just know"
> > that the code is black formatted, or apply the patch, run black, and
> > see if the formatting changes.
> 
> The use of black would be documented somewhere (in the repo, in the
> wiki, or both).  Nevertheless, I don't expect all contributors to know
> about it and do it.
> 
> I also don't expect reviewers to be able to spot code that isn't
> black-compliant.  If you think of checking it or asking about it in a
> review, great.  Otherwise, I don't think we should be too afraid of
> errors slipping in, since they're trivial to fix.
> 
> But to be frank, as a reviewer today, I don't really know what to check
> in the Python code formatting.  Our wiki:
> 
>   https://sourceware.org/gdb/wiki/Internals%20GDB-Python-Coding-Standards
> 
> says that we follow PEP8.  I have no idea how to format PEP8 code, so
> as a result I'm not really checking the formatting of Python code today.
> 
> > Then of course, there's the questions about what happens when
> > different users are running different versions of black - assuming
> > different versions might format things slightly differently.
> 
> Indeed, I should have mentioned this: we would define a version to use,
> just like we do for autoconf/automake.
> 
> The goal of black is that the output won't change just for fun, but it
> could change due to fixed bugs.
> 
> > Another concern I'd have is that unrelated "formatting" changes might
> > creep in.
> > 
> > So, I edit a .py file and either don't run black (naughty), or use
> > version X of black.
> > 
> > Much later, I edit a different part of the same file, now either I run
> > black, or run version Y of black.  My original edit is reformatted
> > slightly differently.
> > 
> > My assumption is that this change (the second patch) should be
> > rejected at review, as the reformatting change (of the earlier code)
> > is unrelated to the work of the second patch.  But I suspect some folk
> > might find it frustrating, if on one had we say run black, but on the
> > other hand we say you need to exclude unrelated chunks.
> 
> This is why I think it's easier to start from a clean state, by
> formatting everything in one shot in the beginning.
> 
> If the issue you describe happens while I am writing a patch, I would
> notice the spurious diff and push an obvious "Format foo.py with
> black" commit and rebase my WIP patch on top.
> 
> If the issue you describe happens while I am revieweing a patch, I would
> probably also push an obvious re-formatting commit (or ask the person to
> do so, if they have push access).  The offending hunk will go away when
> they rebase, which is necessary prior to pushing.
> 
> > I think, in the heterogeneous environments we all develop in,
> > situations like the above will crop up, so it would be important to
> > know what the expectations would be in such a case.
> 
> Indeed.  I hope I was able to show that these situations can be handled
> easily and are not a showstopper.

Thanks for taking the time to reply.

I have no objections to adopting the use of black, my only request
would be that we formally make it a policy that "rogue" re-formatting
hunks, as we discussed above, should be fixed in a separate commit,
and not included in random patches.

For me, one of the great things about working on GDB is the generally
good quality of the commits, and I feel that if commits start
including extra reformatting this would be a step backwards.

> 
> >>
> >> If we want to be a bit more sophisticated, we could check-in a git
> >> commit hook that runs black (if available) to check your formatting as
> >> you are committing, if your commit includes changes to Python files.
> >> That doesn't prevents formatting errors from slipping in the repo, but
> >> it's already a good start (and formatting errors slipping in the repo
> >> are not the end of the world).
> > 
> > Again, this sort of thing works great in a corporate environment where
> > you can guarantee that everyone is (or has no excuse not to be)
> > running the exact same version of every tool.
> 
> Even in corporate environment, you can't assume that!

I disagree, but I suspect the problem is I didn't explain myself well
enough.

Never mind, given the above I think you've answered my questions.

Thanks for your time,
Andrew


> 
> > My concern would be that such a strategy would invite unintended
> > changes to creep in.
> > 
> >>
> >> We don't really have CI right now (especially pre-merge testing), but if
> >> we had, checking the formatting of Python files could be a validation
> >> step.
> > 
> > Agreed.
> > 
> >> We do however, have scripts that run server side on push, so it would be
> >> possible to run black server-side to reject commits that would introduce
> >> formatting errors.
> > 
> > I guess this would solve a lot of the problems I'm concerned about.
> > Once we've "corrected" the formatting of all .py code then nobody
> > could merge bad code.
> 
> Thanks,
> 
> Simon

Re: Proposal: format GDB Python files with black

[Simon Marchi]

> I have no objections to adopting the use of black, my only request
> would be that we formally make it a policy that "rogue" re-formatting
> hunks, as we discussed above, should be fixed in a separate commit,
> and not included in random patches.
> 
> For me, one of the great things about working on GDB is the generally
> good quality of the commits, and I feel that if commits start
> including extra reformatting this would be a step backwards.

I agree.  I thought it was kind of obvious, because it's a continuation
of what we do today: if somebody includes an unrelated formatting change
in a patch, you'll tell them about it.  But it's true that the risk of
it happening with an automated is perhaps greater, as people will run
the tool and not carefully check the output.  I happen to carefully
check the diff of my patches before sending them, but maybe not everyone
does that.

So I'll make sure to include that in the "rules to follow" for
re-formatting.

> Never mind, given the above I think you've answered my questions.
> 
> Thanks for your time,

Thanks for the questions, you raised good points I overlooked.

Simon

Re: Proposal: format GDB Python files with black

[Joel Brobecker]

> Simon> A few people I talked to about this and I have good experience with the
> Simon> tool black to auto-format Python code.
> 
> Yeah, +1 from me.  We use it internally here and it has been fine.  And,
> it eliminates an entire review step, which is good.

Same for me. And I also recomment the "pre-commit" python program
to manage the pre-commit hooks. Both Tom and I should be able to
help with that, if you like.

-- 
Joel

Re: Proposal: format GDB Python files with black

[Tom Tromey]

>>>>> "Lancelot" == Lancelot SIX via Gdb-patches <gdb-patches@sourceware.org> writes:

Lancelot> Another option can be to use 'black' to
Lancelot> help format the code and 'pycodestyle'[3] to do the automated checks,
Lancelot> but I am not sure this is the way you want to go to, introducing more
Lancelot> and more tools.

Doing some kind of checking like this would be good as well.
We've used pycodestyle internally a bit, but recently (-ish) moved to
flake8.  I don't know why we made the switch or which static checker is
preferable, though.

Lancelot> I’d also like to point out that by default, 'black' will use 88 char
Lancelot> long lines[4], which is not compliant with the PEP8[5], which is, as you
Lancelot> pointed out[1], referenced in GDB's wiki[6].  Either the tool's
Lancelot> configuration or the standard can be adjusted, depending on the
Lancelot> maintainers preferences.

We only really chose PEP8 because it existed; not out of some deeper
liking for it or anything like that.  So personally I'm fine with just
switching to a tool.

Tom

Re: Proposal: format GDB Python files with black

[Simon Marchi]

On 2021-04-30 1:04 p.m., Tom Tromey wrote:
>>>>>> "Lancelot" == Lancelot SIX via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Lancelot> Another option can be to use 'black' to
> Lancelot> help format the code and 'pycodestyle'[3] to do the automated checks,
> Lancelot> but I am not sure this is the way you want to go to, introducing more
> Lancelot> and more tools.
> 
> Doing some kind of checking like this would be good as well.
> We've used pycodestyle internally a bit, but recently (-ish) moved to
> flake8.  I don't know why we made the switch or which static checker is
> preferable, though.


I have used flake8 for a long time, I'm happy with it.  It does find
some real bugs, like a variable being referenced without being assigned,
things like that, so it has real value.

> Lancelot> I’d also like to point out that by default, 'black' will use 88 char
> Lancelot> long lines[4], which is not compliant with the PEP8[5], which is, as you
> Lancelot> pointed out[1], referenced in GDB's wiki[6].  Either the tool's
> Lancelot> configuration or the standard can be adjusted, depending on the
> Lancelot> maintainers preferences.
> 
> We only really chose PEP8 because it existed; not out of some deeper
> liking for it or anything like that.  So personally I'm fine with just
> switching to a tool.

Although PEP8 specifies more than just formatting, like naming
convention and things like that.  So for the wiki, which mentions PEP8
currently, I plan on saying something like "Use black for the
formatting.  For the rest, follow PEP8.".

Let's just use black's default line length.  I certainly don't want to
start a discussion on line length, and I don't see any reason to deviate
from the default.

Simon

Re: Proposal: format GDB Python files with black

[Luis Machado]

On 4/26/21 12:55 PM, Simon Marchi via Gdb-patches wrote:
> Hi all,
> 
> A few people I talked to about this and I have good experience with the
> tool black to auto-format Python code.  It is simple to use, fast and
> reliable (if it detects that it changed the meaning of the program by
> mistake, internal-errors out).  I don't think I need to spell out the
> advantage of using such a tool, but just in case: it removes all the
> overhead of thinking about formatting when writing or reviewing code.
> 
> My suggestion would be to format our code with black all at once.  The
> typical counter-argument to this is "it will be harder to use
> git-blame".  I don't think this is true, because you need to be able to
> skip over commits anyway, and it's trivial to skip over a commit when
> git-blaming using an interactive tool.  But it's also possible to tell
> git to ignore certain commits when git-blaming [2], so we can do that.
> 
> I think the output of black is quite readable.  When it does weird
> stuff, it's generally because the lines / expressions are two long
> anyway, and deserve to be split in multiple lines / expressions.  Here's
> a branch that shows how it would look like:
> 
>    https://sourceware.org/git/?p=binutils-gdb.git;a=shortlog;h=refs/heads/users/simark/black
> 
> If the feedback is overall positive, I'll send a more format patch.
> 
> Simon
> 
> [1] https://github.com/psf/black
> [2] https://github.com/psf/black#migrating-your-code-style-without-ruining-git-blame
> 

FTR, I like this too.

Re: Proposal: format GDB Python files with black

[Joel Brobecker]

> > Doing some kind of checking like this would be good as well.
> > We've used pycodestyle internally a bit, but recently (-ish) moved to
> > flake8.  I don't know why we made the switch or which static checker is
> > preferable, though.

flake8 does more, as it is a wrapper around pyflakes and pycodestyle.

> 
> I have used flake8 for a long time, I'm happy with it.  It does find
> some real bugs, like a variable being referenced without being assigned,
> things like that, so it has real value.
> 
> > Lancelot> I’d also like to point out that by default, 'black' will use 88 char
> > Lancelot> long lines[4], which is not compliant with the PEP8[5], which is, as you
> > Lancelot> pointed out[1], referenced in GDB's wiki[6].  Either the tool's
> > Lancelot> configuration or the standard can be adjusted, depending on the
> > Lancelot> maintainers preferences.
> > 
> > We only really chose PEP8 because it existed; not out of some deeper
> > liking for it or anything like that.  So personally I'm fine with just
> > switching to a tool.
> 
> Although PEP8 specifies more than just formatting, like naming
> convention and things like that.  So for the wiki, which mentions PEP8
> currently, I plan on saying something like "Use black for the
> formatting.  For the rest, follow PEP8.".
> 
> Let's just use black's default line length.  I certainly don't want to
> start a discussion on line length, and I don't see any reason to deviate
> from the default.

Agreed.

One other thing that I thought about is perhaps we can look at
writing a server-side hook that would be run at commit time which
would run those checks before accepting them. The git-hooks we are
using provide support for adding extra checks via the
"hooks.commit-extra-checker" option. I'm not 100% about how easy
or difficult it would be to write the actual script that would
reproduce the issue, though.

-- 
Joel

Re: Proposal: format GDB Python files with black

[Tom Tromey]

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

My github has my current hacks, on the t/clang-format branch.  This
branch just has .clang-format, I didn't reformat the sources.  I'm using
clang-format 10.0.1.

I managed to tweak the penalty values in the format to avoid the
bin-packing problem I was seeing previously.  I haven't extensively
tested this (I usually only reformat gdb_bfd.c and look at it, since it
exercised several bugs before...), so I recommend that others play with
it a bit.

I did see a few issues, though perhaps they are minor.

1. It removes all the Control-L page breaks from the source.
   I know these often confuse newcomers, so maybe it's fine.
   I feel that Someone out there will cheer this :)

2. clang-format can't handle GNU-style goto label out-denting.
   There's a clang-format bug on this topic
   https://bugs.llvm.org/show_bug.cgi?id=24118

3. In gdb if we have a large 'if' condition that consists of expression
   chained with "&&" or "||", we often put each sub-condition on its own
   line.  clang-format packs them instead, and I didn't see a way to
   counteract this.

4. Gettext invocations like _("text") have a space added, like _ ("text").
   I didn't see a way to change this.

Another possible problem is that, unlike with 'black', ensuring that
everyone has the same version is a pain.

Maybe we could live with these?  Let me know what you think.
I can continue my experiments if this seems worthwhile.

Tom

Re: Proposal: format GDB Python files with black

[Simon Marchi]

Changing the subject, so this gets more visibility.

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.

> My github has my current hacks, on the t/clang-format branch.  This
> branch just has .clang-format, I didn't reformat the sources.  I'm using
> clang-format 10.0.1.
> 
> I managed to tweak the penalty values in the format to avoid the
> bin-packing problem I was seeing previously.  I haven't extensively
> tested this (I usually only reformat gdb_bfd.c and look at it, since it
> exercised several bugs before...), so I recommend that others play with
> it a bit.

I formatted the whole code base and checked a few files in Meld.  In
general, I would say that it does more good (fixing little mistakes,
finding split lines that could actually be one line) than harm.  I
didn't see anything outrageously badly formatted (and if there is, we
always have the option to disable formatting for a section of a file).

There are a lot of changes that are neutral, where the before and after
would be equally good.

And of course, some changes where it does things not exactly like we
would, like:

  gcc_type
  compile_cplus_instance::convert_reference_base
    (gcc_type base, enum gcc_cp_ref_qualifiers rquals)

which becomes:

  gcc_type
  compile_cplus_instance::convert_reference_base (
    gcc_type base, enum gcc_cp_ref_qualifiers rquals)

But really, I don't think these should be blockers.  It would take
someone some really bad faith to say that the above changes how they
read the code.

Is the following what you don't like?  In gdbsupport/common-debug.h,
before:

		debug_prefixed_printf (m_module, m_func, "%s: <%s debugging was not enabled on entry>",
				       m_end_prefix, m_module);

after:

		debug_prefixed_printf (
		  m_module, m_func,
		  "%s: <%s debugging was not enabled on entry>", m_end_prefix,
		  m_module);

Note: the current line is too long, my bad.

I have only seen this when we make clang-format's life difficult by
using long names or long string literals.  If we don't like how it
formats some piece of code, we always have the choice of tweaking it to
make its like easier.  It can be a good motivation to try to reduce
indentation levels by splitting the code into functions, for example.

I'm surprised you didn't use "UseTab: Always", that gives pretty much
our scheme of tabs + spaces for indentation.  By default it uses just
spaces (I wouldn't mind using either, but since the goal is to get
something close to our current style).

I would use "IndentPPDirectives: AfterHash".  Unlike what we use
currently, it will indent with $IndentWidth columns, whereas we
typically we indent these with a single column when we do it manually.
But, I think I prefer 2 columns actually.  You can try formatting
gdbsupport/gdb_locale.h for a good example.

Speaking of which, in order to be usable from gdbsupport and gdbserver,
.clang-format would need to be in the top-level directory.  Or maybe it
could be in gdbsupport, and gdb/gdbserver symlink to it.

We have a number of places that put the namespace brace on the same
line:

  namespace foo {

We could keep that by using "BreakBeforeBraces: Custom".  But I don't
think it's worth the trouble.  It adds one extra line for the opening
brace, but it's typically not in a region of the file that you care much
about.

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.

For AlignEscapedNewlines, I'd probably choose Right or DontAlign.  The
reason being that if you modify the longest line of a macro (make that
line longer or shorter), all the macros line will change as a result.
So it will be harder to spot what changed when reading the hunk in the
diff.  I have a slight preference for Right, because it puts the
backslashes out of sight, thus improving readability.  It's the kind of
thing that is quite annoying to maintain by hand, but it's not if a tool
does it.

I would suggest setting SplitEmptyFunction, SplitEmptyRecord and
SplitEmptyNamespace to false.  We already do it for empty methods.

Enough for now.

> I did see a few issues, though perhaps they are minor.
> 
> 1. It removes all the Control-L page breaks from the source.
>    I know these often confuse newcomers, so maybe it's fine.
>    I feel that Someone out there will cheer this :)

Who dat?

> 
> 2. clang-format can't handle GNU-style goto label out-denting.
>    There's a clang-format bug on this topic
>    https://bugs.llvm.org/show_bug.cgi?id=24118

Probably not a big deal for us, as we don't use goto much?

> 
> 3. In gdb if we have a large 'if' condition that consists of expression
>    chained with "&&" or "||", we often put each sub-condition on its own
>    line.  clang-format packs them instead, and I didn't see a way to
>    counteract this.

I hate too long conditions anyway.  If the condition spans too many
lines or has deep parenthesis nesting, it would deserve to be refactored
anyway.  Like, put that code in a function and just have:

  if (my_new_function (...))

With a descriptive name for the function, the code ends up more readable.

> 4. Gettext invocations like _("text") have a space added, like _ ("text").
>    I didn't see a way to change this.

A very little inconvenience IMO, compared to not having to think about
formatting.  We could live with this, and file a bug at the same time.
There could be an option to add exceptions to SpaceBeforeParens.

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

Another question is: would we re-format the C/C++ code in the testsuite?
It might require a few adjustments to test cases (like there was one for
black), but I would be fine with that.  My concern is more if there are
some tests where the formatting of the code was specifically important.
We could disable formatting for them, but the difficulty is to find
them.

Simon

Using clang-format

[Simon Marchi]

On 2021-05-10 10:55 p.m., Simon Marchi via Gdb-patches wrote:
> Changing the subject, so this gets more visibility.

Arf, I forgot to do that before sending.  There, done.

> 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.
> 
>> My github has my current hacks, on the t/clang-format branch.  This
>> branch just has .clang-format, I didn't reformat the sources.  I'm using
>> clang-format 10.0.1.
>>
>> I managed to tweak the penalty values in the format to avoid the
>> bin-packing problem I was seeing previously.  I haven't extensively
>> tested this (I usually only reformat gdb_bfd.c and look at it, since it
>> exercised several bugs before...), so I recommend that others play with
>> it a bit.
> 
> I formatted the whole code base and checked a few files in Meld.  In
> general, I would say that it does more good (fixing little mistakes,
> finding split lines that could actually be one line) than harm.  I
> didn't see anything outrageously badly formatted (and if there is, we
> always have the option to disable formatting for a section of a file).
> 
> There are a lot of changes that are neutral, where the before and after
> would be equally good.
> 
> And of course, some changes where it does things not exactly like we
> would, like:
> 
>   gcc_type
>   compile_cplus_instance::convert_reference_base
>     (gcc_type base, enum gcc_cp_ref_qualifiers rquals)
> 
> which becomes:
> 
>   gcc_type
>   compile_cplus_instance::convert_reference_base (
>     gcc_type base, enum gcc_cp_ref_qualifiers rquals)
> 
> But really, I don't think these should be blockers.  It would take
> someone some really bad faith to say that the above changes how they
> read the code.
> 
> Is the following what you don't like?  In gdbsupport/common-debug.h,
> before:
> 
> 		debug_prefixed_printf (m_module, m_func, "%s: <%s debugging was not enabled on entry>",
> 				       m_end_prefix, m_module);
> 
> after:
> 
> 		debug_prefixed_printf (
> 		  m_module, m_func,
> 		  "%s: <%s debugging was not enabled on entry>", m_end_prefix,
> 		  m_module);
> 
> Note: the current line is too long, my bad.
> 
> I have only seen this when we make clang-format's life difficult by
> using long names or long string literals.  If we don't like how it
> formats some piece of code, we always have the choice of tweaking it to
> make its like easier.  It can be a good motivation to try to reduce
> indentation levels by splitting the code into functions, for example.
> 
> I'm surprised you didn't use "UseTab: Always", that gives pretty much
> our scheme of tabs + spaces for indentation.  By default it uses just
> spaces (I wouldn't mind using either, but since the goal is to get
> something close to our current style).
> 
> I would use "IndentPPDirectives: AfterHash".  Unlike what we use
> currently, it will indent with $IndentWidth columns, whereas we
> typically we indent these with a single column when we do it manually.
> But, I think I prefer 2 columns actually.  You can try formatting
> gdbsupport/gdb_locale.h for a good example.
> 
> Speaking of which, in order to be usable from gdbsupport and gdbserver,
> .clang-format would need to be in the top-level directory.  Or maybe it
> could be in gdbsupport, and gdb/gdbserver symlink to it.
> 
> We have a number of places that put the namespace brace on the same
> line:
> 
>   namespace foo {
> 
> We could keep that by using "BreakBeforeBraces: Custom".  But I don't
> think it's worth the trouble.  It adds one extra line for the opening
> brace, but it's typically not in a region of the file that you care much
> about.
> 
> 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.
> 
> For AlignEscapedNewlines, I'd probably choose Right or DontAlign.  The
> reason being that if you modify the longest line of a macro (make that
> line longer or shorter), all the macros line will change as a result.
> So it will be harder to spot what changed when reading the hunk in the
> diff.  I have a slight preference for Right, because it puts the
> backslashes out of sight, thus improving readability.  It's the kind of
> thing that is quite annoying to maintain by hand, but it's not if a tool
> does it.
> 
> I would suggest setting SplitEmptyFunction, SplitEmptyRecord and
> SplitEmptyNamespace to false.  We already do it for empty methods.
> 
> Enough for now.
> 
>> I did see a few issues, though perhaps they are minor.
>>
>> 1. It removes all the Control-L page breaks from the source.
>>    I know these often confuse newcomers, so maybe it's fine.
>>    I feel that Someone out there will cheer this :)
> 
> Who dat?
> 
>>
>> 2. clang-format can't handle GNU-style goto label out-denting.
>>    There's a clang-format bug on this topic
>>    https://bugs.llvm.org/show_bug.cgi?id=24118
> 
> Probably not a big deal for us, as we don't use goto much?
> 
>>
>> 3. In gdb if we have a large 'if' condition that consists of expression
>>    chained with "&&" or "||", we often put each sub-condition on its own
>>    line.  clang-format packs them instead, and I didn't see a way to
>>    counteract this.
> 
> I hate too long conditions anyway.  If the condition spans too many
> lines or has deep parenthesis nesting, it would deserve to be refactored
> anyway.  Like, put that code in a function and just have:
> 
>   if (my_new_function (...))
> 
> With a descriptive name for the function, the code ends up more readable.
> 
>> 4. Gettext invocations like _("text") have a space added, like _ ("text").
>>    I didn't see a way to change this.
> 
> A very little inconvenience IMO, compared to not having to think about
> formatting.  We could live with this, and file a bug at the same time.
> There could be an option to add exceptions to SpaceBeforeParens.
> 
>> 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.
> 
> Another question is: would we re-format the C/C++ code in the testsuite?
> It might require a few adjustments to test cases (like there was one for
> black), but I would be fine with that.  My concern is more if there are
> some tests where the formatting of the code was specifically important.
> We could disable formatting for them, but the difficulty is to find
> them.
> 
> Simon
> 

Re: Proposal: format GDB Python files with black

[Luis Machado]

On 5/10/21 11:55 PM, Simon Marchi via Gdb-patches wrote:
> Changing the subject, so this gets more visibility.
> 
> 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.
> 

I'm fine with using a tool to autoformat things. I wouldn't mind getting 
used to a slightly different formatting scheme and I feel it would save 
me quite a bit of time by not having to bother with little formatting 
details.

>> My github has my current hacks, on the t/clang-format branch.  This
>> branch just has .clang-format, I didn't reformat the sources.  I'm using
>> clang-format 10.0.1.
>>
>> I managed to tweak the penalty values in the format to avoid the
>> bin-packing problem I was seeing previously.  I haven't extensively
>> tested this (I usually only reformat gdb_bfd.c and look at it, since it
>> exercised several bugs before...), so I recommend that others play with
>> it a bit.
> 
> I formatted the whole code base and checked a few files in Meld.  In
> general, I would say that it does more good (fixing little mistakes,
> finding split lines that could actually be one line) than harm.  I
> didn't see anything outrageously badly formatted (and if there is, we
> always have the option to disable formatting for a section of a file).
> 
> There are a lot of changes that are neutral, where the before and after
> would be equally good.
> 
> And of course, some changes where it does things not exactly like we
> would, like:
> 
>    gcc_type
>    compile_cplus_instance::convert_reference_base
>      (gcc_type base, enum gcc_cp_ref_qualifiers rquals)
> 
> which becomes:
> 
>    gcc_type
>    compile_cplus_instance::convert_reference_base (
>      gcc_type base, enum gcc_cp_ref_qualifiers rquals)
> 
> But really, I don't think these should be blockers.  It would take
> someone some really bad faith to say that the above changes how they
> read the code.
> 
> Is the following what you don't like?  In gdbsupport/common-debug.h,
> before:
> 
> 		debug_prefixed_printf (m_module, m_func, "%s: <%s debugging was not enabled on entry>",
> 				       m_end_prefix, m_module);
> 
> after:
> 
> 		debug_prefixed_printf (
> 		  m_module, m_func,
> 		  "%s: <%s debugging was not enabled on entry>", m_end_prefix,
> 		  m_module);
> 
> Note: the current line is too long, my bad.
> 
> I have only seen this when we make clang-format's life difficult by
> using long names or long string literals.  If we don't like how it
> formats some piece of code, we always have the choice of tweaking it to
> make its like easier.  It can be a good motivation to try to reduce
> indentation levels by splitting the code into functions, for example.
> 
> I'm surprised you didn't use "UseTab: Always", that gives pretty much
> our scheme of tabs + spaces for indentation.  By default it uses just
> spaces (I wouldn't mind using either, but since the goal is to get
> something close to our current style).
> 
> I would use "IndentPPDirectives: AfterHash".  Unlike what we use
> currently, it will indent with $IndentWidth columns, whereas we
> typically we indent these with a single column when we do it manually.
> But, I think I prefer 2 columns actually.  You can try formatting
> gdbsupport/gdb_locale.h for a good example.
> 
> Speaking of which, in order to be usable from gdbsupport and gdbserver,
> .clang-format would need to be in the top-level directory.  Or maybe it
> could be in gdbsupport, and gdb/gdbserver symlink to it.
> 
> We have a number of places that put the namespace brace on the same
> line:
> 
>    namespace foo {
> 
> We could keep that by using "BreakBeforeBraces: Custom".  But I don't
> think it's worth the trouble.  It adds one extra line for the opening
> brace, but it's typically not in a region of the file that you care much
> about.
> 
> 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.
> 
> For AlignEscapedNewlines, I'd probably choose Right or DontAlign.  The
> reason being that if you modify the longest line of a macro (make that
> line longer or shorter), all the macros line will change as a result.
> So it will be harder to spot what changed when reading the hunk in the
> diff.  I have a slight preference for Right, because it puts the
> backslashes out of sight, thus improving readability.  It's the kind of
> thing that is quite annoying to maintain by hand, but it's not if a tool
> does it.
> 
> I would suggest setting SplitEmptyFunction, SplitEmptyRecord and
> SplitEmptyNamespace to false.  We already do it for empty methods.
> 
> Enough for now.
> 
>> I did see a few issues, though perhaps they are minor.
>>
>> 1. It removes all the Control-L page breaks from the source.
>>     I know these often confuse newcomers, so maybe it's fine.
>>     I feel that Someone out there will cheer this :)
> 
> Who dat?
> 
>>
>> 2. clang-format can't handle GNU-style goto label out-denting.
>>     There's a clang-format bug on this topic
>>     https://bugs.llvm.org/show_bug.cgi?id=24118
> 
> Probably not a big deal for us, as we don't use goto much?
> 
>>
>> 3. In gdb if we have a large 'if' condition that consists of expression
>>     chained with "&&" or "||", we often put each sub-condition on its own
>>     line.  clang-format packs them instead, and I didn't see a way to
>>     counteract this.
> 
> I hate too long conditions anyway.  If the condition spans too many
> lines or has deep parenthesis nesting, it would deserve to be refactored
> anyway.  Like, put that code in a function and just have:
> 
>    if (my_new_function (...))
> 
> With a descriptive name for the function, the code ends up more readable.
> 
>> 4. Gettext invocations like _("text") have a space added, like _ ("text").
>>     I didn't see a way to change this.
> 
> A very little inconvenience IMO, compared to not having to think about
> formatting.  We could live with this, and file a bug at the same time.
> There could be an option to add exceptions to SpaceBeforeParens.
> 
>> 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.

I suppose we could have scripts to automate this sort of task. Something 
that checks for the latest stable release and downloads it? A helper to 
make things easier and more consistent.

Can we do this server-side and take the burden off of the developers to 
have to do this as an additional step before pushing changes?

> 
> 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.
> 
> Another question is: would we re-format the C/C++ code in the testsuite?
> It might require a few adjustments to test cases (like there was one for
> black), but I would be fine with that.  My concern is more if there are
> some tests where the formatting of the code was specifically important.
> We could disable formatting for them, but the difficulty is to find
> them.

I think reformatting the test sources has the potential to break things 
on testcases with hardcoded assumptions. I wouldn't touch them at first.

> 
> Simon
> 

Re: Using clang-format

[Marco Barisione]

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

Re: Using clang-format

[Simon Marchi]

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

Thanks for that feedback.

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

Tom's config has

  BreakConstructorInitializers: BeforeColon

But it still does it.

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

I think that's a good option.  It's a bit of manual steps, but:

1. It's O(1), it's as quick to do this with a small patch and a big
   patch.
2. It's temporary, eventually all patches that were in the pipeline at
   the time of the re-format will be submitted or dropped.

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

Agreed.

Simon

Re: Proposal: format GDB Python files with black

[Simon Marchi]

On 2021-05-11 7:38 a.m., Luis Machado wrote:
>> 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.
> 
> I suppose we could have scripts to automate this sort of task. Something that checks for the latest stable release and downloads it? A helper to make things easier and more consistent.

Download from where?

> 
> Can we do this server-side and take the burden off of the developers to have to do this as an additional step before pushing changes?

We can check server-side if the formatting is good and reject the commit
if not, but I don't think we can modify the commit being pushed (I don't
think it would be a good idea if that was possible).

But just like black (it's on my todo list to check that), we can have a
commit hook that runs clang-format (at least just to warn you, "Hey, you
code needs to be formatted" if clang-format would change something).

Simon

Re: Proposal: format GDB Python files with black

[Luis Machado]

On 5/11/21 10:49 AM, Simon Marchi wrote:
> On 2021-05-11 7:38 a.m., Luis Machado wrote:
>>> 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.
>>
>> I suppose we could have scripts to automate this sort of task. Something that checks for the latest stable release and downloads it? A helper to make things easier and more consistent.
> 
> Download from where?
> 

I guess this works better for black since you can fetch it from a repo 
and use a particular version.

Can we do something similar for clang-format without having to build the 
entire LLVM codebase?

>>
>> Can we do this server-side and take the burden off of the developers to have to do this as an additional step before pushing changes?
> 
> We can check server-side if the formatting is good and reject the commit
> if not, but I don't think we can modify the commit being pushed (I don't
> think it would be a good idea if that was possible).

I see. Maybe a client-side hook that formats things during a local 
commit will work better then.

That way we can send a local commit for review and it will be properly 
formatted already.

> 
> But just like black (it's on my todo list to check that), we can have a
> commit hook that runs clang-format (at least just to warn you, "Hey, you
> code needs to be formatted" if clang-format would change something).

I guess that would as well.

Re: Using clang-format

[Tom Tromey]

>> gcc_type
>> compile_cplus_instance::convert_reference_base (
>> gcc_type base, enum gcc_cp_ref_qualifiers rquals)
>> 
>> But really, I don't think these should be blockers.  It would take
>> someone some really bad faith to say that the above changes how they
>> read the code.

I don't agree, mainly because I dislike that style, and I feel like I'm
generally above-board.

This weird dangling paren was an issue for calls, previously, though,
and I fixed it by tweaking one of the penalty settings.  So maybe that
can be salvaged.

>> I have only seen this when we make clang-format's life difficult by
>> using long names or long string literals.  If we don't like how it
>> formats some piece of code, we always have the choice of tweaking it to
>> make its like easier.

Yeah.  Also it's not like the current code is pristine.

>> I'm surprised you didn't use "UseTab: Always"

I routinely forget whether gdb uses tabs or not and rely on Emacs to
DTRT.

>> I would use "IndentPPDirectives: AfterHash".  Unlike what we use
>> currently, it will indent with $IndentWidth columns, whereas we
>> typically we indent these with a single column when we do it manually.
>> But, I think I prefer 2 columns actually.  You can try formatting
>> gdbsupport/gdb_locale.h for a good example.

My impression was that normally gdb didn't do this kind of indentation,
but in this particular case, I don't really have any opinion and I'm
happy with whatever setting everyone else enjoys.

>> Speaking of which, in order to be usable from gdbsupport and gdbserver,
>> .clang-format would need to be in the top-level directory.  Or maybe it
>> could be in gdbsupport, and gdb/gdbserver symlink to it.

Yeah, I was wondering about this as well.

I don't think we should put it at top-level, though, as people may think
it applies to the whole tree, which it wouldn't.

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

I think clang-format doesn't offer this level of control here.

>> For AlignEscapedNewlines, I'd probably choose Right or DontAlign.  The
>> reason being that if you modify the longest line of a macro (make that
>> line longer or shorter), all the macros line will change as a result.
>> So it will be harder to spot what changed when reading the hunk in the
>> diff.  I have a slight preference for Right,

Sounds good.

>>> 1. It removes all the Control-L page breaks from the source.
>>> I know these often confuse newcomers, so maybe it's fine.
>>> I feel that Someone out there will cheer this :)
>> 
>> Who dat?

I thought Pedro was in the anti-control-L camp?
Anyway, no biggie, I like them but I probably won't notice their
absence.

>>> Another possible problem is that, unlike with 'black', ensuring that
>>> everyone has the same version is a pain.

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

If our reformatting commit is purely automated, we can write a short
script using git filter-branch so that anybody with a patch to rebase
can first rebase to the commit just before the reformat, then run the
script to rebase-and-reformat.  This would ease the pain somewhat.

>> Another question is: would we re-format the C/C++ code in the testsuite?
>> It might require a few adjustments to test cases (like there was one for
>> black), but I would be fine with that.  My concern is more if there are
>> some tests where the formatting of the code was specifically important.
>> We could disable formatting for them, but the difficulty is to find
>> them.

Personally I would say that we shouldn't touch the tests at all, and
just not worry about the formatting there.  That's been my usual review
policy, as I think it's fine, and maybe even mildly beneficial in some
theoretical way, for the tests to have a variety of styles.  Also
normally they are just throwaway code anyway.

Tom

Re: Using clang-format

[Simon Marchi]

On 2021-05-11 4:40 p.m., Tom Tromey wrote:
>>> gcc_type
>>> compile_cplus_instance::convert_reference_base (
>>> gcc_type base, enum gcc_cp_ref_qualifiers rquals)
>>>
>>> But really, I don't think these should be blockers.  It would take
>>> someone some really bad faith to say that the above changes how they
>>> read the code.
> 
> I don't agree, mainly because I dislike that style, and I feel like I'm
> generally above-board.

Ok, and sorry about my original formulation, when re-reading it it
sounds a bit strong.

Simon