Re: automated formatting

[David Kastrup]

Han-Wen Nienhuys <address@hidden> writes:

> On Mon, Jan 27, 2020 at 11:49 AM David Kastrup <address@hidden> 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