1643561 - C++ formatting in comm-central - finish switching clang-format rules over to mozilla-central style.

Status: RESOLVED FIXED

(MailNews Core :: General, task)

Description

[Ben Campbell]

There's one C++ style difference left between c-c and m-c. The pointer/reference grouping, eg:
in c-c:

void foo(nsIThingy *thing) {

in m-c:

void foo(nsIThingy* thing) {

We're 99% there already (Bug 1546364). But c-c still has a custom .clang-format.
And if we're going to do it, it seems like the time is now, otherwise it'll complicate backporting security fixes for the next release.

I've done a test run over the whole codebase, excluding the third_party dir, and the calendar and ldap C libraries, ie:

./mach clang-format -p comm/calendar/base
./mach clang-format -p comm/common
./mach clang-format -p comm/ldap/xpcom
./mach clang-format -p comm/mail
./mach clang-format -p comm/mailnews

It touches about 524 files and the patch comes to about 6MB.
Eyeballing the patch, it seems pretty sane - just moving the pointers/references about.

I'll do a try build next, then we decide if we want to pull the trigger.

Comment 1

[Ben Campbell]

Compiles and passes unit tests locally. Try build has some mochitest fails which I'm not sure about:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=ad913c2e0675cc917d8dc92b7072255be9e2b57e

(if the reformatting caused problems, I'd expect them to be in the form of compilation fails rather than runtime fails).

Comment 2

[Ben Campbell]

Oh, forgot to add: there are a few C++ files in suite/. Should I include them in the reformat or not? Not sure who to ask.

Comment 3

[Frank-Rainer Grahl (:frg)]

Oh, forgot to add: there are a few C++ files in suite/. Should I include them in the reformat or not? Not sure who to ask.

Ben, thanks for asking. Currently not.. The reformats ruin the history / blame and we are still using 2 older trees for uplifts. We will do it piecemeal and the remaining formatting changes then later.

Comment 4

[Jorg K (CEST = GMT+2)]

I'm not sure who decided to move the stars to the left when it would be a lot less churn to move them all to the right, or just leave them where they are since the current rule is based on prevalence. See bug 1546364 comment #30 and above. If at all, this should have been done at the end of the 78 cycle, like the other reformatting was done at the end of the 68 cycle.

There is a religious war going on about where to put the stars and I see no benefit in taking sides.

EDIT: The patch on try is missing the # ignore-this-changeset line. Apparently what can be used to ignore this changeset in the blame.

Comment 5

[Ben Campbell]

The aim was to ditch the c-c specific .clang-format and use the m-c rules unchanged.

I think this is a worthwhile goal although in practice it's not toooo big a deal (other than the minor niggles of Bug 1575449).
But if it's going to be a hassle for 78 maybe it should be pushed back until the next window of opportunity?

Comment 6

[Magnus Melin [:mkmelin]]

(In reply to Jorg K (CEST = GMT+2) from comment #4)

If at all, this should have been done at the end of the 78 cycle, like the other reformatting was done at the end of the 68 cycle.

The number of c++ changes since branching is very small (and most likely such we should uplift), so now is fairly good time still.

There is a religious war going on about where to put the stars and I see no benefit in taking sides.

What we have now is inconsistent. It's inconsistent with mozilla-central, it's inconsistent with it self just by looking at the file next to it! The whole point of using formatting is to get consistency, which clearly was not achieved yet.

Comment 7

[Ben Campbell]

OK, try runs before and after performing the reformat look fine, so I'm going to land it.

Before:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=33c8e95a6c3c3e00e2be0bd518610e94197aece6
After:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=03cce772c1e3ed411a218763d12c25bff477df2b

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/06085479b243
Reformat C++ to M-C clang-format rules (mainly PointerAlignment: Left). rs=mkmelin

Comment 9

[Ben Campbell]

Just an extra note: I added a .clang-format-ignore file to exclude third_party libraries and the like from clang-format runs.
But due to the way mach clang-format currently operates, it's unlikely to be picked up (see Bug 1575449).
So for now, consider it documentation rather than anything enforced.

Comment 10

[Jorg K (CEST = GMT+2)]

You should put this on comm-beta to avoid endless uplift chagrin. You should also remove ldap-sdk from the ignore file since it was reformatted and should stay that way, examples:
https://hg.mozilla.org/comm-central/log/tip/ldap/c-sdk/libraries/liblber/bprint.c
https://hg.mozilla.org/comm-central/log/tip/ldap/c-sdk/libraries/liblber/encode.c
https://hg.mozilla.org/comm-central/log/tip/ldap/c-sdk/libraries/libldap/abandon.c

The library/SDK is so far behind the upstream source that we decided to treat the code as our own. Uff, you'll have to reshuffle stars there too now :-(

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/7e4d2d1cab11
Apply M-C clang-format rules to ldap c-sdk. rs=mkmelin

Comment 12

[Ben Campbell]

Try build for the ldap reformat:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=fc0937393edeb8a3547631e43ca2a08dd261daaf

Comment 13

[Ben Campbell]

Attachment: 1643561-ignore-revs-1.patch

Comment 14

[Ben Campbell]

Attachment: 1643561-clang-format-ignore-remove-ldap-1.patch

Comment 15

[Jorg K (CEST = GMT+2)]

Hmm, looks like you didn't follow comment #4.

EDIT: The patch on try is missing the # ignore-this-changeset line. Apparently what can be used to ignore this changeset in the blame.

Comment 16

[Magnus Melin [:mkmelin]]

Comment on attachment 9158408 [details] [diff] [review]
1643561-ignore-revs-1.patch

Review of attachment 9158408 [details] [diff] [review]:
-----------------------------------------------------------------

You could change the order so they are listed last one landed last

Comment 17

[Ben Campbell]

Attachment: 1643561-ignore-revs-2.patch

updated for chronological order.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/c7233f867599
Remove ldap c-sdk from .clang-format-ignore list. r=mkmelin
https://hg.mozilla.org/comm-central/rev/5b14ad062ce8
Add C++ reformatting changesets to .hg-annotate-ignore-revs. r=mkmelin

Comment 19

[Magnus Melin [:mkmelin]]

If this is ready to land on beta, it could go into the last beta, scheduled for Mon 29 (US time). If not we could defer it to .1 (along with bug 1647104)

Comment 20

[Ben Campbell]

(In reply to Magnus Melin [:mkmelin] from comment #19)

If this is ready to land on beta, it could go into the last beta, scheduled for Mon 29 (US time). If not we could defer it to .1 (along with bug 1647104)

I've just finished building a beta locally, with the two big reformatting patches, and it looks good. Runs and passes the unit tests locally.
There were 10 or 12 conflicts to manually resolve (due to a non-beta nsIArray-removal patch), but nothing too tricky. Presumably I'd just land it on comm-beta directly as I've already rebased it? (pending approval, of course)

Comment 21

[Ben Campbell]

Oh.
No file attactments to request approval-comm-beta on...

• Can I just get an approval in a comment here from someone, and add them as an a=whoever in the changeset?
• There are two patches, one for ldap, one for everything else. Is it best to leave them separate or to merge them?
• can/should I be running a comm-beta try build first? If so, how does that work?

Comment 22

[Magnus Melin [:mkmelin]]

You can needinfo Wayne.
Please leave the patches separate.
A beta try run can be created by editing .gecko_rev.yml and fixing it to the appropriate beta changeset. A try run would be good yes.

Comment 23

[Jorg K (CEST = GMT+2)]

How much time do you want to waste with approvals here for a patch that essentially changes some white-space? It is already evening in NZ and the US day shift will do the merges and land the last part of bug 1647104. Magnus has approval power, so I suggest to go ahead and push this to beta now. BTW, putting this onto beta was already agreed and there isn't a lot of risk management necesary.

Comment 24

[Ben Campbell]

Try build running here:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=2890cc6368dd3d5efab6a94e7bcbfe07f53ce8aa

If the try run is good and Wayne is happy to approve the patches, then they can be grabbed directly from the try repo rather than waiting for me to land them tomorrow.

Comment 25

[Jorg K (CEST = GMT+2)]

And the equivalents of the changesets in comment #18? At least the first one? Ah well, not no important.

Comment 26

[Rob Lemley [:rjl]]

Thunderbird 78.0b4:
https://hg.mozilla.org/releases/comm-beta/rev/9c08c80ec341
https://hg.mozilla.org/releases/comm-beta/rev/70337d494218
https://hg.mozilla.org/releases/comm-beta/rev/8b4996912204
https://hg.mozilla.org/releases/comm-beta/rev/fde9f21875e3

Comment 27

[Wayne Mery (:wsmwk)]

Thanks for handling this