Clean up xpcom in preparation for clang-format

RESOLVED FIXED in Firefox -esr60

Status

()

enhancement
RESOLVED FIXED
6 months ago
5 months ago

People

(Reporter: mccr8, Assigned: mccr8)

Tracking

(Blocks 1 bug)

unspecified
mozilla65
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr60 fixed, firefox65 fixed)

Details

Attachments

(10 attachments)

Assignee

Description

6 months ago
There's a number of changes that Clang format makes in the xpcom/ directory that look bad, mostly around how string literals or comments are wrapped. I think by making them fit within 80 columns we can avoid that.
Assignee

Comment 1

6 months ago
For instance, the formatting for the large about:memory descriptions look bad:

-      MOZ_COLLECT_REPORT(
-        "private", KIND_OTHER, UNITS_BYTES, amount,
-"Memory that cannot be shared with other processes, including memory that is "
-"committed and marked MEM_PRIVATE, data that is not mapped, and executable "
-"pages that have been written to.");
+      MOZ_COLLECT_REPORT("private", KIND_OTHER, UNITS_BYTES, amount,
+                         "Memory that cannot be shared with other processes, "
+                         "including memory that is "
+                         "committed and marked MEM_PRIVATE, data that is not "
+                         "mapped, and executable "
+                         "pages that have been written to.");

There are also a few other little things to improve, like some tables in nsCRTGlue.cpp that get weird.
Assignee

Comment 2

6 months ago
Here's a weird one, from xpcom/ds/nsEnumeratorUtils.cpp:

-  // nsISimpleEnumerator
-  NS_DECL_NSISIMPLEENUMERATOR
-  NS_DECL_NSIUTF8STRINGENUMERATOR
-  NS_DECL_NSISTRINGENUMERATORBASE
-  // can't use NS_DECL_NSISTRINGENUMERATOR because they share the
-  // HasMore() signature
-  NS_IMETHOD GetNext(nsAString& aResult) override;
+      // nsISimpleEnumerator
+      NS_DECL_NSISIMPLEENUMERATOR NS_DECL_NSIUTF8STRINGENUMERATOR
+          NS_DECL_NSISTRINGENUMERATORBASE
+              // can't use NS_DECL_NSISTRINGENUMERATOR because they share the
+              // HasMore() signature
+              NS_IMETHOD
+              GetNext(nsAString& aResult) override;

It seems to think that the NS_DECL things are part of the declaration of GetNext. Adding in blank lines doesn't help.

The thing before this in the file is:
  NS_DECL_ISUPPORTS_INHERITED  // not really inherited, but no mRefCnt

If you move the comment to its own line, then it fixes it...
Indeed, it is hard for clang-format to differentiate macro than normal functions.

If this workaround works, we should probably do that, don't you think?
Assignee

Comment 4

6 months ago
(In reply to Sylvestre Ledru [:sylvestre] from comment #3)
> If this workaround works, we should probably do that, don't you think?

I'm not sure what you mean by this. I filed this bug because I was planning on applying the workaround.
Assignee

Comment 6

6 months ago
Clang format makes this code look pretty bad, but I think it is safe
to just remove it.

Depends on D13182
Assignee

Comment 7

6 months ago
Clang format does not always reflow comments correctly to get them
within 80 columns.

The major categories of failures I have noticed in xpcom/ are:

- Comments that are lists. I fixed these by manually getting them so
  they'll be within 80 columns after clang-format runs.

- Comments intermixed with lists of things like enums, initializers,
  or even fields in a class. It doesn't seem to associate the comment
  with the item in the list correctly. The worst cases of these happen
  when it changes initializer lists from having commas at the start of
  each item to having them at the end. In the initializer comma cases,
  I fixed them by making the commas at the end, so clang-format won't
  mix things up. For other cases, I often moved the comment for an
  item onto its own line, because it was not possible to have both the
  comment and the item on the same line and stay within 80 columns.

- One odd case is nsEnumeratorUtils.cpp, where the end of line comment
  after a NS_DECL macro confused clang-format and made it stop
  realizing that the NS_DECL thing was a complete statement. I also
  added a blank line to that file before a declaration because I think
  that is better.

Depends on D13183
Assignee

Comment 8

6 months ago
clang-format doesn't seem to reflow text when a single string constant
is represented as multiple lines of string literals to fit within 80
columns. Instead, if a single string literal is too long, it breaks
off a piece and moves it to the next line, which leads to a bunch of
choppy short string bits. The resulting string literals are still a
bit choppy, mostly because I didn't want to break up the long names of
prefs.

Here's an example of what I mean:

|--- max width --- |
"some long string literal"
"is here but it goes on for multiple lines"

This gets turned into:

|--- max width --- |
"some long string "
"literal"
"is here but it "
"goes on for multiple lines"

My patch manually would turn this into:

|--- max width --- |
"some long string "
"literal is here "
"but it goes on "
"for multiple lines"

The strings in my patch don't always end up at column 80, because the
width is set to work with wherever clang format ends up actually
indenting them.

There are more instances of this problem when MOZ_COLLECT_REPORT is
used, but that can be dealt with in another bug. There are a ton of
them.

Depends on D13184
Assignee

Comment 9

6 months ago
nsCRTGlue.cpp has a few data tables that get broken up badly by
clang-format, so just disable the formatting. There are some more
tables in this file, but the arrangement doesn't seem to matter for
them.

The bloat log header is just a little bit of ASCII art, so just ignore
it in formatting so it is easier to read.

Depends on D13185

Comment 10

6 months ago
Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1da65c8d52d3
part 1 - Add some missing things to the clang-format macro list. r=Ehsan

Comment 11

6 months ago
(I landed the first part to ensure that it's definitely in the tree in time...)
Keywords: leave-open
Assignee

Comment 12

6 months ago
(I set the rest of the patches to land in Lando, but the tree is closed now.)

Comment 14

6 months ago
Pushed by amccreight@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b226d4c40893
part 2 - Remove DEBUG_rickg from nsDeque.cpp. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/67b1d1e43301
part 3 - Shrink comments so they don't get poorly reflowed by clang-format. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/09c2de91a87f
part 4 - Rearrange various string literals so they fit in 80 columns. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/d53add8e4130
part 5 - Disable Clang formatting in a few places in xpcom/. r=froydnj

Updated

6 months ago
Keywords: leave-open

Comment 16

5 months ago
[ESR Uplift Approval Request]

If this is not a sec:{high,crit} bug, please state case for ESR consideration: This is required for easier backporting of patches after the reformatting of ESR using clang-format.

User impact if declined: Declining this will negatively impact our developers' ability to easily backport their patches to ESR in the future.

Fix Landed on Version: 65

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): This is NPOTB.

String or UUID changes made by this patch: None
Attachment #9031005 - Flags: approval-mozilla-esr60?

Comment 17

5 months ago
[ESR Uplift Approval Request]

If this is not a sec:{high,crit} bug, please state case for ESR consideration: 

User impact if declined: 

Fix Landed on Version: 

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): This patch removes some dead code.

String or UUID changes made by this patch:
Attachment #9031006 - Flags: approval-mozilla-esr60?

Comment 18

5 months ago
[ESR Uplift Approval Request]

If this is not a sec:{high,crit} bug, please state case for ESR consideration: 

User impact if declined: 

Fix Landed on Version: 

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): This changes whitespace and comments only.

String or UUID changes made by this patch:
Attachment #9031007 - Flags: approval-mozilla-esr60?

Comment 19

5 months ago
[ESR Uplift Approval Request]

If this is not a sec:{high,crit} bug, please state case for ESR consideration: 

User impact if declined: 

Fix Landed on Version: 

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): This patch breaks some string literals into two.  Shouldn't change anything in the binaries we ship.

String or UUID changes made by this patch:
Attachment #9031008 - Flags: approval-mozilla-esr60?

Comment 20

5 months ago
[ESR Uplift Approval Request]

If this is not a sec:{high,crit} bug, please state case for ESR consideration: 

User impact if declined: 

Fix Landed on Version: 

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): This patch is comment only changes.

String or UUID changes made by this patch:
Attachment #9031009 - Flags: approval-mozilla-esr60?
Comment on attachment 9031009 [details] [diff] [review]
ESR patch (part 5)

OK for uplift to ESR for clang-format project.
Attachment #9031009 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
Attachment #9031005 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
Attachment #9031008 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
Attachment #9031006 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
Attachment #9031007 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
You need to log in before you can comment on or make changes to this bug.