automated formatting

classic Classic list List threaded Threaded
14 messages Options
Reply | Threaded
Open this post in threaded view
|

automated formatting

Han-Wen Nienhuys-3
I want to propose to move to automated formatting for our C++ code.

I put up a .clang-format code that mostly mimicks our style at

  https://codereview.appspot.com/561340043

I have a lot of good experience with automating code formatting.. It
removes drudgery for code authors, obviates discussions over style in
code review, and generally elevates the level of discourse in our
reviews.

What do you all think?

The current config modifies about 11k lines, mostly because of
different line breaks (necessary to keep the 80 column limit.)

If anyone wants to tinker more, see
https://clang.llvm.org/docs/ClangFormatStyleOptions.html
for further options.

Obviously, reformatting code makes patches harder to transport, so
we'd have to do it on all active branches at the same time.

--
Han-Wen Nienhuys - [hidden email] - http://www.xs4all.nl/~hanwen

Reply | Threaded
Open this post in threaded view
|

Re: automated formatting

Werner LEMBERG

> I put up a .clang-format code that mostly mimicks our style at
>
>   https://codereview.appspot.com/561340043
>
> I have a lot of good experience with automating code formatting.  It
> removes drudgery for code authors, obviates discussions over style
> in code review, and generally elevates the level of discourse in our
> reviews.

I like that, thanks!

> The current config modifies about 11k lines, mostly because of
> different line breaks (necessary to keep the 80 column limit.)

As soon as I'm able to run this, I will check the formatting in
detail.

The 80 column limit is a special beast.  Sometimes it *does* make
sense to not follow this restriction, especially if the excess is just
a few characters...

> Obviously, reformatting code makes patches harder to transport, so
> we'd have to do it on all active branches at the same time.

I suggest to do such a clean-up after the releases of both 2.20 and
2.21.


    Werner

Reply | Threaded
Open this post in threaded view
|

Re: automated formatting

David Kastrup
In reply to this post by Han-Wen Nienhuys-3
Han-Wen Nienhuys <[hidden email]> writes:

> I want to propose to move to automated formatting for our C++ code.
>
> I put up a .clang-format code that mostly mimicks our style at
>
>   https://codereview.appspot.com/561340043
>
> I have a lot of good experience with automating code formatting.. It
> removes drudgery for code authors, obviates discussions over style in
> code review, and generally elevates the level of discourse in our
> reviews.
>
> What do you all think?

scripts/auxiliar/fixcc.py and astyle 2.04 is what we standardized on.

> The current config modifies about 11k lines, mostly because of
> different line breaks (necessary to keep the 80 column limit.)

Any particular reason to change the automated style to a different one?
Clang is a pretty big dependency for developers.

> If anyone wants to tinker more, see
> https://clang.llvm.org/docs/ClangFormatStyleOptions.html
> for further options.
>
> Obviously, reformatting code makes patches harder to transport, so
> we'd have to do it on all active branches at the same time.

--
David Kastrup

Reply | Threaded
Open this post in threaded view
|

Re: automated formatting

David Kastrup
David Kastrup <[hidden email]> writes:

> Han-Wen Nienhuys <[hidden email]> writes:
>
>> I want to propose to move to automated formatting for our C++ code.
>>
>> I put up a .clang-format code that mostly mimicks our style at
>>
>>   https://codereview.appspot.com/561340043
>>
>> I have a lot of good experience with automating code formatting.. It
>> removes drudgery for code authors, obviates discussions over style in
>> code review, and generally elevates the level of discourse in our
>> reviews.
>>
>> What do you all think?
>
> scripts/auxiliar/fixcc.py and astyle 2.04 is what we standardized on.
>
>> The current config modifies about 11k lines, mostly because of
>> different line breaks (necessary to keep the 80 column limit.)
>
> Any particular reason to change the automated style to a different one?
> Clang is a pretty big dependency for developers.

I just put up an issue that makes the --sloppy option of fixcc.py work
(but updating to 3.04 does not seem like much of an issue either)

>> Obviously, reformatting code makes patches harder to transport, so
>> we'd have to do it on all active branches at the same time.

That's why I think changing the existing automated formatting style is
an unwanted complication.

In Graham's reign, rerunning the C++ formatter was a pretty frequent
occurence, I think it was more than once yearly.  We also have a Scheme
code formatter that relies on Emacs to do the hard work.

Frankly, I just forgot about either.  I think it would be a good idea to
take up this practice again.  Regarding Clang: unless Astyle falls apart
using C++11 constructs that are going to occur more frequently in
future, I think there is little reason to depart from its use instead of
reverting to Clang.

--
David Kastrup

Reply | Threaded
Open this post in threaded view
|

Re: automated formatting

Dan Eble
On Jan 27, 2020, at 07:43, David Kastrup <[hidden email]> wrote:
>
> Frankly, I just forgot about either.  I think it would be a good idea to
> take up this practice again.  Regarding Clang: unless Astyle falls apart
> using C++11 constructs that are going to occur more frequently in
> future, I think there is little reason to depart from its use instead of
> reverting to Clang.

I have learned not to fuss about the particulars of required style as long as I have the following:

1. a simple way to exclude sections of code manually

2. a style check integrated into "make check"

3. a makefile target that automatically restyles all source so that "make check" will pass.  Only the files that need to be modified are touched.  This runs only when requested, never as a prerequisite of any other target.

Dan


Reply | Threaded
Open this post in threaded view
|

Re: automated formatting

Han-Wen Nienhuys-3
In reply to this post by David Kastrup
On Mon, Jan 27, 2020 at 11:49 AM David Kastrup <[hidden email]> wrote:

> > I want to propose to move to automated formatting for our C++ code.
> >
> > I put up a .clang-format code that mostly mimicks our style at
> >
> >   https://codereview.appspot.com/561340043
> >
> > I have a lot of good experience with automating code formatting.. It
> > removes drudgery for code authors, obviates discussions over style in
> > code review, and generally elevates the level of discourse in our
> > reviews.
> >
> > What do you all think?
>
> scripts/auxiliar/fixcc.py and astyle 2.04 is what we standardized on.

Yes, I am proposing that we change and standardize on clang-format.

> > The current config modifies about 11k lines, mostly because of
> > different line breaks (necessary to keep the 80 column limit.)
>
> Any particular reason to change the automated style to a different one?

Several reasons

* clang-format is a *complete* formatter. It reformats files to a
canonical formatting regardless of how badly you mangle the input.

For example, if you introduce a comment line of 200 chars. In
clang-format, this will be reformatted to the specified column limit.
Astyle/fixcc doesn't know what to do with it.

* clang-format is based on clang itself, and has an understanding of
the source code being formatted. This makes it better than fixcc which
uses regular expressions (sic) to make sense of C++. This is
especially important as we move towards newer dialects of  C++ with
newer constructs (eg. lambda).

* Because it is so robust, it is safe to run automatically on save,
and there exists VIM and Emacs support to do so

* fixcc is 500 lines of python that we could just stop maintaining.

* clang-format is fast. I can format all of the LilyPond C++ code in 4
seconds. That makes it fast enough to stick into a pre-commit hook
(check changed files for formatting).

* clang-format is configurable. I whipped a config that I think
matches what we have, but I might have missed some options. Ideally,
the changes would be small.

* clang-format is well-established. Both the linux kernel and Git have
a .clang-format files in their trees.

Finally, commenting on Werner: there are two main positives for
automated formatting:

1. you can stop caring about formatting complely, ie.

> The 80 column limit is a special beast.  Sometimes it *does* make
> sense to not follow this restriction, especially if the excess is just
> a few characters...

when the formatting is completely automatic, there is "follow" or "not
follow". You simply press "Save" and the code is formatted as it is
supposed to be. Maybe that means the result is slightly suboptimal in
specific situations, but overall, not having to think about formatting
is very liberating.

2. When code has standard formatting, it is much easier to build
automation for code changes, or do global search/replaces over the
code base. (This is probably not a big factor for LilyPond, but it's
what makes large scale automated refactoring feasible at places like
Google.)

> Clang is a pretty big dependency for developers.

You'd think that, but it's not that bad:

$ rpm -qi clang| grep Size
Size        : 1695283

--
Han-Wen Nienhuys - [hidden email] - http://www.xs4all.nl/~hanwen

Reply | Threaded
Open this post in threaded view
|

Re: automated formatting

David Kastrup
Han-Wen Nienhuys <[hidden email]> writes:

> On Mon, Jan 27, 2020 at 11:49 AM David Kastrup <[hidden email]> wrote:
>> > I want to propose to move to automated formatting for our C++ code.
>> >
>> > I put up a .clang-format code that mostly mimicks our style at
>> >
>> >   https://codereview.appspot.com/561340043
>> >
>> > I have a lot of good experience with automating code formatting.. It
>> > removes drudgery for code authors, obviates discussions over style in
>> > code review, and generally elevates the level of discourse in our
>> > reviews.
>> >
>> > What do you all think?
>>
>> scripts/auxiliar/fixcc.py and astyle 2.04 is what we standardized on.
>
> Yes, I am proposing that we change and standardize on clang-format.
>
>> > The current config modifies about 11k lines, mostly because of
>> > different line breaks (necessary to keep the 80 column limit.)
>>
>> Any particular reason to change the automated style to a different one?
>
> Several reasons
>
> * clang-format is a *complete* formatter. It reformats files to a
> canonical formatting regardless of how badly you mangle the input.

Do we want to mangle the input badly?

> For example, if you introduce a comment line of 200 chars. In
> clang-format, this will be reformatted to the specified column limit.
> Astyle/fixcc doesn't know what to do with it.

Comments often contain ASCII-art and formatted tables.  Formatting those
is not helpful.

> * clang-format is based on clang itself, and has an understanding of
> the source code being formatted. This makes it better than fixcc which
> uses regular expressions (sic) to make sense of C++. This is
> especially important as we move towards newer dialects of  C++ with
> newer constructs (eg. lambda).

C++11 support is already in the release notes for Astyle 2.03 in 2013.

> * Because it is so robust, it is safe to run automatically on save,
> and there exists VIM and Emacs support to do so

I don't understand what you call "safe" and "robust" here.  Do you
suggest that using Astyle would change the meaning of C++ code?

> * fixcc is 500 lines of python that we could just stop maintaining.

Fixcc does not much more than drive Astyle, and Astyle is being
maintained by someone else.  Here is its dependency list for the sake of
people using lily-dev or similar:

Depends: libc6 (>= 2.14), libstdc++6 (>= 5.2)

Here is the dependency list of clang-format:

Depends: libc6 (>= 2.14), libclang-cpp9, libgcc1 (>= 1:3.0), libllvm9 (= 1:9-2), libstdc++6 (>= 5.2), python3

So one dependency is python3, another is libclang-cpp9, and then there
are various other compiler parts (both GCC and LLVM).

I have put up an issue showing the current job fixcc/astyle would do on
our code base.  Do you see significant problems showing up there?

> * clang-format is fast. I can format all of the LilyPond C++ code in 4
> seconds. That makes it fast enough to stick into a pre-commit hook
> (check changed files for formatting).

time scripts/auxiliar/fixcc.py --sloppy $(git ls-files '*.cc' '*.hh')

real 0m12.651s
user 0m10.593s
sys 0m2.057s

and I'd be willing to bet that your laptop runs faster than mine with
its Intel(R) Core(TM) i7-2630QM CPU @ 2.00GHz .

> * clang-format is configurable. I whipped a config that I think
> matches what we have, but I might have missed some options. Ideally,
> the changes would be small.

The changes for AStyle are small because we already use AStyle.

> * clang-format is well-established. Both the linux kernel and Git have
> a .clang-format files in their trees.

AStyle is well-established since we have been using it for years without
a problem.  If we get problems, that may warrant reevaluation.

Can you point out problems in the sample I posted as

Current branch: issue5703
Tracker issue: 5703 (https://sourceforge.net/p/testlilyissues/issues/5703/)
Rietveld issue: 549480043 (https://codereview.appspot.com/549480043)
Issue description:
  Run scripts/auxiliar/fixcc.py  This is just intended to showcase the
  current fixes fixcc would cause.

?

> Finally, commenting on Werner: there are two main positives for
> automated formatting:

I am not going to argue here since that is an old discussion with a
resolution way in the past under Graham's tenure, and we opted for it.
I forgot about the proper upkeep, and your reminder is very much
appreciated.  But we do have a working solution right now, even if it is
my fault not to have minded it properly, and changing to one with vastly
more dependencies and having to redefine formatting from scratch is
something that I cannot, in the current situation, see as a winning
proposition.

In particular, pinning the operation to a particular version of Clang
and making it automatic in an editor would make it a considerable
headache for people to _not_ accidentally stomp back and forth over the
formatting of files when their Clang version does not match that of
other people.

> 2. When code has standard formatting, it is much easier to build
> automation for code changes, or do global search/replaces over the
> code base. (This is probably not a big factor for LilyPond, but it's
> what makes large scale automated refactoring feasible at places like
> Google.)

Actually, indentation changes are a nightmare for version control.
That's why we decided already in the past to do them in batches.

>> Clang is a pretty big dependency for developers.
>
> You'd think that, but it's not that bad:
>
> $ rpm -qi clang| grep Size
> Size        : 1695283

And how many dependencies are there in it?

dak@lola:/usr/local/tmp/lilypond$ apt-cache depends clang-9
clang-9
  Depends: libc6
  Depends: libclang-cpp9
  Depends: libgcc1
  Depends: libllvm9
  Depends: libstdc++6
  Depends: libstdc++-8-dev
  Depends: libgcc-8-dev
  Depends: libobjc-8-dev
  Depends: libclang-common-9-dev
  Depends: libclang1-9
  Depends: libc6-dev
  Depends: binutils
  Recommends: llvm-9-dev
  Recommends: python3
  Recommends: libomp-9-dev
  Suggests: clang-9-doc
dak@lola:/usr/local/tmp/lilypond$ apt-cache depends astyle
astyle
  Depends: libc6
  Depends: libstdc++6
  Suggests: xdg-utils


--
David Kastrup

Reply | Threaded
Open this post in threaded view
|

Re: automated formatting

David Kastrup
David Kastrup <[hidden email]> writes:

> Han-Wen Nienhuys <[hidden email]> writes:
>
>> On Mon, Jan 27, 2020 at 11:49 AM David Kastrup <[hidden email]> wrote:
>>> > I want to propose to move to automated formatting for our C++ code.
>>> >
>>> > I put up a .clang-format code that mostly mimicks our style at
>>> >
>>> >   https://codereview.appspot.com/561340043
>>> >
>>> > I have a lot of good experience with automating code formatting.. It
>>> > removes drudgery for code authors, obviates discussions over style in
>>> > code review, and generally elevates the level of discourse in our
>>> > reviews.
>>> >
>>> > What do you all think?
>>>
>>> scripts/auxiliar/fixcc.py and astyle 2.04 is what we standardized on.
>>
>> Yes, I am proposing that we change and standardize on clang-format.
>>
>>> > The current config modifies about 11k lines, mostly because of
>>> > different line breaks (necessary to keep the 80 column limit.)
>>>
>>> Any particular reason to change the automated style to a different one?
>>
>> Several reasons
>>
>> * clang-format is a *complete* formatter. It reformats files to a
>> canonical formatting regardless of how badly you mangle the input.
>
> Do we want to mangle the input badly?
>
>> For example, if you introduce a comment line of 200 chars. In
>> clang-format, this will be reformatted to the specified column limit.
>> Astyle/fixcc doesn't know what to do with it.
>
> Comments often contain ASCII-art and formatted tables.  Formatting those
> is not helpful.

Here are examples:

  /*
   * this case (distant half collide),
   *
   *    |
   *  x |
   * | x
   * |
   *
   * the noteheads may be closer than this case (close half collide)
   *
   *    |
   *    |
   *   x
   *  x
   * |
   * |
   *
   */


              /* TODO:

              For some cases we should kern some more: when the
              distance between the next or prev note is too large, we'd
              get large white gaps, eg.

              |
              X|
              |X  <- kern this.
              |
              X

              */


>
>> * clang-format is based on clang itself, and has an understanding of
>> the source code being formatted. This makes it better than fixcc which
>> uses regular expressions (sic) to make sense of C++.

For better or worse, we still do a lot with preprocessor macros that are
applied in the manner of declarations and statements.  They are
formatted in a superficial manner, reflecting the kind of construct they
are mimicking (declarations, function calls) for the sake of human
readers.  Reflecting the underlying C++ realities instead would be a
distraction.  I don't want to diss Clang here: it is unlikely that it
would actually dig down to the C++ level for analysing these constructs
instead of staying at the preprocessor level.  But what I am saying is
that we usually want to make code readable to _humans_ and that means
patterning it in a manner where deep semantic analysis should not be a
requirement.

I understand that it was chiefly my neglect of our existing reformatting
regime that caused you to look for a solution for a problem
unnecessarily and invest time into it.  I apologize for that, and am
willing to bear the cost in future cherry-picks into the 2.20 stable
branch for reformatting both 2.20.0 and 2.21.0 before release.

But the additional cost for changing to a completely different
reformatting system just does not seem warranted as long as there are no
obvious deficiencies to be seen with what we have now.

--
David Kastrup

Reply | Threaded
Open this post in threaded view
|

Re: automated formatting

Han-Wen Nienhuys-3
On Tue, Jan 28, 2020 at 12:18 AM David Kastrup <[hidden email]> wrote:

>
> David Kastrup <[hidden email]> writes:
>
> > Han-Wen Nienhuys <[hidden email]> writes:
> >
> >> On Mon, Jan 27, 2020 at 11:49 AM David Kastrup <[hidden email]> wrote:
> >>> > I want to propose to move to automated formatting for our C++ code.
> >>> >
> >>> > I put up a .clang-format code that mostly mimicks our style at
> >>> >
> >>> >   https://codereview.appspot.com/561340043
> >>> >
> >>> > I have a lot of good experience with automating code formatting.. It
> >>> > removes drudgery for code authors, obviates discussions over style in
> >>> > code review, and generally elevates the level of discourse in our
> >>> > reviews.
> >>> >
> >>> > What do you all think?
> >>>
> >>> scripts/auxiliar/fixcc.py and astyle 2.04 is what we standardized on.
> >>
> >> Yes, I am proposing that we change and standardize on clang-format.
> >>
> >>> > The current config modifies about 11k lines, mostly because of
> >>> > different line breaks (necessary to keep the 80 column limit.)
> >>>
> >>> Any particular reason to change the automated style to a different one?
> >>
> >> Several reasons
> >>
> >> * clang-format is a *complete* formatter. It reformats files to a
> >> canonical formatting regardless of how badly you mangle the input.
> >
> > Do we want to mangle the input badly?
> >
> >> For example, if you introduce a comment line of 200 chars. In
> >> clang-format, this will be reformatted to the specified column limit.
> >> Astyle/fixcc doesn't know what to do with it.
> >
> > Comments often contain ASCII-art and formatted tables.  Formatting those
> > is not helpful.
>
> Here are examples:
>
>   /*
>    * this case (distant half collide),
>    *
>    *    |
>    *  x |
>    * | x
>    * |
>    *
>    * the noteheads may be closer than this case (close half collide)
>    *
>    *    |
>    *    |
>    *   x
>    *  x
>    * |
>    * |
>    *
>    */
>
>
>               /* TODO:
>
>               For some cases we should kern some more: when the
>               distance between the next or prev note is too large, we'd
>               get large white gaps, eg.
>
>               |
>               X|
>               |X  <- kern this.
>               |
>               X
>
>               */
>
>
> >
> >> * clang-format is based on clang itself, and has an understanding of
> >> the source code being formatted. This makes it better than fixcc which
> >> uses regular expressions (sic) to make sense of C++.
>
> For better or worse, we still do a lot with preprocessor macros that are
> applied in the manner of declarations and statements.  They are
> formatted in a superficial manner, reflecting the kind of construct they
> are mimicking (declarations, function calls) for the sake of human
> readers.  Reflecting the underlying C++ realities instead would be a
> distraction.  I don't want to diss Clang here: it is unlikely that it
> would actually dig down to the C++ level for analysing these constructs
> instead of staying at the preprocessor level.  But what I am saying is
> that we usually want to make code readable to _humans_ and that means
> patterning it in a manner where deep semantic analysis should not be a
> requirement.
>
> I understand that it was chiefly my neglect of our existing reformatting
> regime that caused you to look for a solution for a problem
> unnecessarily and invest time into it.  I apologize for that, and am
> willing to bear the cost in future cherry-picks into the 2.20 stable
> branch for reformatting both 2.20.0 and 2.21.0 before release.
>
> But the additional cost for changing to a completely different
> reformatting system just does not seem warranted as long as there are no
> obvious deficiencies to be seen with what we have now.

I think you are missing the larger point I have been trying to make.

In Salzburg, people asked "what scares contributors away", and "what
can we do to suck Han-Wen/Mike back into the project"?

The project cares about code formatting, as evidenced by fixcc.py.
However, having to deal with code formatting, both while writing code,
and during review, is an absolute turn-off for me. If you want to suck
me (and others) in, you have to make the code formatting process as
transparent and automatic as possible.

Can you provide me with a presubmit hook that applies fixcc? Can you
guarantee that, if fixcc has run on the code, there will be no further
remarks on code formatting during review?

The point with completely automatic formatting is that you can simply
add a rule to "make check" (or some sort of "check" if we were to use
github or gitlab) that prevents the code ever to stray from the
defined standard.

I know that Dan also has issues with the current code formatting process.

If you reject clang-format (on grounds that I don't agree with, BTW),
then can you come up with another solution that lets me (and other
contributors) stop worrying about formatting in some other way? Note
that I can't even run fixcc (see below).

BTW, If you want to peruse clang-format output, you can go to
https://review.gerrithub.io/c/hanwen/lilypond/+/483012. As you can see
in

https://review.gerrithub.io/c/hanwen/lilypond/+/483012/1/lily/note-collision.cc

the ASCII art is unscathed. However, the file shows several problems
that fixcc didn't bother to fix (note-collision.cc is not in your
fixcc demo change.)

My machine is a 2012 T420 (i5 2540M), which is considerably slower than yours

$ time python2 scripts/auxiliar/fixcc.py --sloppy $(git ls-files '*.cc' '*.hh')
..
getopt.GetoptError: option --sloppy not recognized

$ time python2 scripts/auxiliar/fixcc.py $(git ls-files '*.cc' '*.hh')
Artistic Style Version 3.1
Warning: try to use Artistic Style Version 2.04.
Please limit use of this version to files with changed code.
Too many files with this version.  See `astyle --help`

Would it help if chatted with you over the telephone? I'm not sure if
we are in agreement of the basic goals we are trying to achieve.

--
Han-Wen Nienhuys - [hidden email] - http://www.xs4all.nl/~hanwen

Reply | Threaded
Open this post in threaded view
|

Re: automated formatting

Carl Sorensen-3


On 1/28/20, 1:50 AM, "lilypond-devel on behalf of Han-Wen Nienhuys" <lilypond-devel-bounces+c_sorensen=[hidden email] on behalf of [hidden email]> wrote:

    On Tue, Jan 28, 2020 at 12:18 AM David Kastrup <[hidden email]> wrote:
    >
    > David Kastrup <[hidden email]> writes:
    >
    > > Han-Wen Nienhuys <[hidden email]> writes:
    > >
    > >> On Mon, Jan 27, 2020 at 11:49 AM David Kastrup <[hidden email]> wrote:
    > >>> > I want to propose to move to automated formatting for our C++ code.
    > >>> >
    > >>> > I put up a .clang-format code that mostly mimicks our style at
    > >>> >
    > >>> >   https://codereview.appspot.com/561340043
    > >>> >
    > >>> > I have a lot of good experience with automating code formatting.. It
    > >>> > removes drudgery for code authors, obviates discussions over style in
    > >>> > code review, and generally elevates the level of discourse in our
    > >>> > reviews.
    > >>> >
    > >>> > What do you all think?
    > >>>
    > >>> scripts/auxiliar/fixcc.py and astyle 2.04 is what we standardized on.
    > >>
    > >> Yes, I am proposing that we change and standardize on clang-format.
    > >>
    > >>> > The current config modifies about 11k lines, mostly because of
    > >>> > different line breaks (necessary to keep the 80 column limit.)
    > >>>
    > >>> Any particular reason to change the automated style to a different one?
    > >>
    > >> Several reasons
    > >>
    > >> * clang-format is a *complete* formatter. It reformats files to a
    > >> canonical formatting regardless of how badly you mangle the input.
    > >
<snip>
    > >
    > >> * clang-format is based on clang itself, and has an understanding of
    > >> the source code being formatted. This makes it better than fixcc which
    > >> uses regular expressions (sic) to make sense of C++.
    >
<snip>
    >
    > I understand that it was chiefly my neglect of our existing reformatting
    > regime that caused you to look for a solution for a problem
    > unnecessarily and invest time into it.  I apologize for that, and am
    > willing to bear the cost in future cherry-picks into the 2.20 stable
    > branch for reformatting both 2.20.0 and 2.21.0 before release.
    >
    > But the additional cost for changing to a completely different
    > reformatting system just does not seem warranted as long as there are no
    > obvious deficiencies to be seen with what we have now.
   
    I think you are missing the larger point I have been trying to make.
   
    In Salzburg, people asked "what scares contributors away", and "what
    can we do to suck Han-Wen/Mike back into the project"?
   
    The project cares about code formatting, as evidenced by fixcc.py.
    However, having to deal with code formatting, both while writing code,
    and during review, is an absolute turn-off for me. If you want to suck
    me (and others) in, you have to make the code formatting process as
    transparent and automatic as possible.

I admit that when I saw a request for clang as a requirement for development I was somewhat dismayed.  Like David, I felt like we had a workable tool, and we could stick with it.

HOWEVER, if moving to clang-format is a price we have to pay to have Han-Wen contributing his expertise to the project, I would willingly pay it.

UNLESS moving to clang-format would drive David K away from contributing his expertise to the project.

I really hope we can find a solution that will satisfy both David and Han-Wen, because I think that both of them are instrumental to the health of LilyPond.  And I love having LilyPond available!
   
    Can you provide me with a presubmit hook that applies fixcc? Can you
    guarantee that, if fixcc has run on the code, there will be no further
    remarks on code formatting during review?

I think that it's a really good idea to have presubmit hooks.  I believe, but can't guarantee, that if all code were automatically reformatted on submission (by either fixcc or clang-format) there would be virtually no comments on formatting.  And you could completely ignore any that were made.
   
    The point with completely automatic formatting is that you can simply
    add a rule to "make check" (or some sort of "check" if we were to use
    github or gitlab) that prevents the code ever to stray from the
    defined standard.
   
I agree that completely automatic formatting is a worthy goal.  I think we should have it.   If we can only have it with clang-format, then we should probably use clang-format.  If we can have it with both clang-format and fixcc, then the decision between the two tools needs to be based on criteria other than completely automatic formatting.

    I know that Dan also has issues with the current code formatting process.
   
    If you reject clang-format (on grounds that I don't agree with, BTW),
    then can you come up with another solution that lets me (and other
    contributors) stop worrying about formatting in some other way? Note
    that I can't even run fixcc (see below).
   
    BTW, If you want to peruse clang-format output, you can go to
    https://review.gerrithub.io/c/hanwen/lilypond/+/483012. As you can see
    in
   
    https://review.gerrithub.io/c/hanwen/lilypond/+/483012/1/lily/note-collision.cc
   
    the ASCII art is unscathed. However, the file shows several problems
    that fixcc didn't bother to fix (note-collision.cc is not in your
    fixcc demo change.)
   
    My machine is a 2012 T420 (i5 2540M), which is considerably slower than yours
   
    $ time python2 scripts/auxiliar/fixcc.py --sloppy $(git ls-files '*.cc' '*.hh')
    ..
    getopt.GetoptError: option --sloppy not recognized
   
    $ time python2 scripts/auxiliar/fixcc.py $(git ls-files '*.cc' '*.hh')
    Artistic Style Version 3.1
    Warning: try to use Artistic Style Version 2.04.
    Please limit use of this version to files with changed code.
    Too many files with this version.  See `astyle --help`

The problem you have with fixcc.py is similar to the problem I would have with clang-format.  It's not set up on my system yet in a way that I can run.  And I'd need to make some changes.  As I said above, I'd be willing to do that.
   
    Would it help if chatted with you over the telephone? I'm not sure if
    we are in agreement of the basic goals we are trying to achieve.

If there is agreement on basic goals, then I'm fully confident that an agreement on the means to achieve the goals can be reached.

Thanks,

Carl
 

Reply | Threaded
Open this post in threaded view
|

Re: automated formatting

Dan Eble
On Jan 28, 2020, at 12:11, Carl Sorensen <[hidden email]> wrote:
>    Can you provide me with a presubmit hook that applies fixcc? Can you
>    guarantee that, if fixcc has run on the code, there will be no further
>    remarks on code formatting during review?
>
> I think that it's a really good idea to have presubmit hooks.  I believe, but can't guarantee, that if all code were automatically reformatted on submission (by either fixcc or clang-format) there would be virtually no comments on formatting.  And you could completely ignore any that were made.

I'm all for making it possible for a contributor to set up something like this if he pleases, but I think it's a bad idea to rely on this level of automation and integration with a specific source-control tool.

I don't trust restyling programs to do the most desirable thing for readability 100% of the time, and I especially don't trust them in certain situations, such as when I'm checking in incomplete work because of an urgent need to switch to another branch.

Also, our basic goal is that none of the cooks poisons the soup, i.e. merges badly formatted code to master.  If some of them want to experiment with wild mushrooms on the side, micromanaging their commits doesn't guard the soup any better than checking when they present their work for integration.

If you build a style check into "make check", then it will happen whenever contributors test their code and it will happen during the review countdown.  If you also build a style check into the final tests before merging staging to master, the goal will be achieved.

Dan


Reply | Threaded
Open this post in threaded view
|

Re: automated formatting

David Kastrup
Dan Eble <[hidden email]> writes:

> On Jan 28, 2020, at 12:11, Carl Sorensen <[hidden email]> wrote:
>>    Can you provide me with a presubmit hook that applies fixcc? Can you
>>    guarantee that, if fixcc has run on the code, there will be no further
>>    remarks on code formatting during review?
>>
>> I think that it's a really good idea to have presubmit hooks.  I
>> believe, but can't guarantee, that if all code were automatically
>> reformatted on submission (by either fixcc or clang-format) there
>> would be virtually no comments on formatting.  And you could
>> completely ignore any that were made.
>
> I'm all for making it possible for a contributor to set up something
> like this if he pleases, but I think it's a bad idea to rely on this
> level of automation and integration with a specific source-control
> tool.
>
> I don't trust restyling programs to do the most desirable thing for
> readability 100% of the time, and I especially don't trust them in
> certain situations, such as when I'm checking in incomplete work
> because of an urgent need to switch to another branch.

To clarify: before you joined the project, there were recurrent
reformatting passes preferably at "quiescent" times to bring consistent
style into the code base.  There were large discussions about it but it
was sort-of agreed that having a uniform style presented to people
reading the code was a reasonably good idea and that the tool chosen and
maintained did not cause noticeable damage (formatting of scm was a more
tricky proposition and the respective script available by now just
defers to Emacs for doing it).

> Also, our basic goal is that none of the cooks poisons the soup,
> i.e. merges badly formatted code to master.  If some of them want to
> experiment with wild mushrooms on the side, micromanaging their
> commits doesn't guard the soup any better than checking when they
> present their work for integration.
>
> If you build a style check into "make check", then it will happen
> whenever contributors test their code and it will happen during the
> review countdown.  If you also build a style check into the final
> tests before merging staging to master, the goal will be achieved.  —

The problem with precommit hooks is that you may end up formatting
unrelated submissions.  Doing it manually, you may end up with
git add -p
in order to just format your own work.

Basically, no real great solution offered itself then (and I don't see
that we can do this significantly better now, even while the particular
tool to use may be subject to debate) and the adapted solution was to
encourage using the fixcc scripts on one's own contribution and
regularly apply it to the repository at reasonable points of time.

I am not sure how far this policy dates back, but while Graham was
project leader, it was done regularly.  There was no intent by me not
following this practice: I simply dropped the ball.

I'd be willing to pick up on it since the adopted compromise was sort of
agreed upon by the active developers.

The set of active developers has changed by now, and of course we also
try angling for the mystic developer just missing one particular
tool/workflow in order to become productive or productive again.  There
is little enough sense in turning this into a showdown rather than an
attempt of finding a consensus everyone can find themselves able to live
with.  I'll do (respectively already have done in a few issues) what I
feel brings the comparatively simple fixcc tool back up to scratch
(fittingly for my non-use of it, its --sloppy option was broken) and
then hopefully we'll be in a better position of deciding how to
continue.

--
David Kastrup

Reply | Threaded
Open this post in threaded view
|

Re: automated formatting

Han-Wen Nienhuys-3
On Tue, Jan 28, 2020 at 8:08 PM David Kastrup <[hidden email]> wrote:

> >>    Can you provide me with a presubmit hook that applies fixcc? Can you
> >>    guarantee that, if fixcc has run on the code, there will be no further
> >>    remarks on code formatting during review?
> >>
> >> I think that it's a really good idea to have presubmit hooks.  I
> >> believe, but can't guarantee, that if all code were automatically
> >> reformatted on submission (by either fixcc or clang-format) there
> >> would be virtually no comments on formatting.  And you could
> >> completely ignore any that were made.
> >
> > I'm all for making it possible for a contributor to set up something
> > like this if he pleases, but I think it's a bad idea to rely on this
> > level of automation and integration with a specific source-control
> > tool.

Sorry, I was unclear. I mean: a local precommit hook (which wouldn't
be mandatory to install)

> To clarify: before you joined the project, there were recurrent
> reformatting passes preferably at "quiescent" times to bring consistent
> style into the code base.  There were large discussions about it but it
> was sort-of agreed that having a uniform style presented to people
> reading the code was a reasonably good idea and that the tool chosen and
> maintained did not cause noticeable damage (formatting of scm was a more
> tricky proposition and the respective script available by now just
> defers to Emacs for doing it).


I ran some of the discussion points by the authors of clang-format
(they work in the Google Munich office.)

Their suggestions:

* formatting can and will change subtly across versions. Difference
will be relatively small, because large users (eg. google) have an
auto-formatted code base, and don't want needless churn on their code
base.

* If there are doubts, it's best to recommend the "diff" option
instead (https://clang.llvm.org/docs/ClangFormat.html#script-for-patch-reformatting),
which reformats diffs, instead of whole files.

* we could introduce an automated check that verifies the diff against
formatting problems. Either as part of the build, in git-cl, or as
suggested pre-commit hook in the docs.

* statically linked binaries of clang-format are available here:
https://github.com/angular/clang-format/tree/master/bin ; we could
standardize on whatever the Angular project recommends (which is v11,
at the moment.)

> Basically, no real great solution offered itself then (and I don't see
> that we can do this significantly better now, even while the particular
> tool to use may be subject to debate) and the adapted solution was to
> encourage using the fixcc scripts on one's own contribution and
> regularly apply it to the repository at reasonable points of time.
>
> I am not sure how far this policy dates back, but while Graham was
> project leader, it was done regularly.  There was no intent by me not
> following this practice: I simply dropped the ball.
>
> I'd be willing to pick up on it since the adopted compromise was sort of
> agreed upon by the active developers.
>
> The set of active developers has changed by now, and of course we also
> try angling for the mystic developer just missing one particular
> tool/workflow in order to become productive or productive again.  There
> is little enough sense in turning this into a showdown rather than an
> attempt of finding a consensus everyone can find themselves able to live
> with.  I'll do (respectively already have done in a few issues) what I
> feel brings the comparatively simple fixcc tool back up to scratch
> (fittingly for my non-use of it, its --sloppy option was broken) and
> then hopefully we'll be in a better position of deciding how to
> continue.

So here is my list of desires, priority from most to least important.
I'd be happy with 1), but I think going to 2) or even 3) would be good
for the LilyPond project as a whole.

1. Recognize clang-format as a "good enough", so I can use it locally
on my diffs and not get reviews on style.

2. Recognize clang-format as recommended, and document how to set it
up for contributors

3. Recognize clang-format as standard, and setup a formatting test on
diffs in the Makefile.

4. Recognize a certain version of clang-format as standard, and setup
a formatting test on the source tree in the Makefile. Users can setup
integration with emacs, vim, CLion, Eclipse etc.

--
Han-Wen Nienhuys - [hidden email] - http://www.xs4all.nl/~hanwen

Reply | Threaded
Open this post in threaded view
|

Re: automated formatting

Werner LEMBERG

> * statically linked binaries of clang-format are available here:
> https://github.com/angular/clang-format/tree/master/bin ;

Aaah!  This is very nice, thanks.

> 1. Recognize clang-format as a "good enough", so I can use it
> locally on my diffs and not get reviews on style.

I did a quick view, and I think the output looks promising; I would
definitely call it 'good enough', and formatting only new code as you
suggested should be easily doable.

The only thing that I dislike is that a situation like

   //   ..... foo bar blabla blubb.
   //   Woodle woodle crunch ...

possibly gets reformatted as

   //   ..... foo bar blabla blubb. Woodle woodle crunch ...
 
this is, a full stop at a line end gets converted to a mid-line full
stop followed by a single space only.

Is there an 'emacs' option for clang-format to get two spaces for such
cases?


    Werner