956199 - Fix style of xpcom/base/nsMemoryReporterManager.{h,cpp}

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Nicholas Nethercote [inactive]]

xpcom/base/nsMemoryReporterManager.{h,cpp} violate standard Gecko style in numerous ways.  Most notabley, nsMemoryReporterManager.cpp mostly uses four-space indents.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Fix style of xpcom/base/nsMemoryReporterManager.{h,cpp} [whitespace changes]

This is just the whitespace changes, which are mostly indentation changes.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: Fix style of xpcom/base/nsMemoryReporterManager.{h,cpp} [other changes]

Non-whitespace changes.  This patch will be merged with the previous one before
landing.

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: Fix style of xpcom/base/nsMemoryReporterManager.{h,cpp} [whitespace changes]

bsmedberg, I'm asking for review on this hand-done style fix, because I don't
feel clang-format is up to snuff for doing large style changes -- it changes
way too many lines that a human would leave alone, causing more churn than
necessary, and it often makes the code uglier (IMO).  (See the attachment
comparing my changes to clang-format's in bug 966840 for examples.)
It also is unable to fix non-whitespace style violations, like misnames
variables and missing braces. Given than, I think entirely hand-done fixes
should be accepted.

This patch contains only the following changes:

- updated the mode line for the new indentation amount
- indentation changes
- some statements that were broken across lines no longer need to be

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: Fix style of xpcom/base/nsMemoryReporterManager.{h,cpp} [other changes]

I'll merge this with the other patch before landing.

Comment 5

[Nicholas Nethercote [inactive]]

Attachment: The first patch, using |diff -w|

This |diff -w| version of the first patch will be much easier to review.

Comment 6

[Benjamin Smedberg]

How did you do the hand-done patch? Entirely by hand, or by using emacs indent-region, or something else?

Comment 7

[Benjamin Smedberg]

Comment on attachment 8375283 [details] [diff] [review]
Fix style of xpcom/base/nsMemoryReporterManager.{h,cpp} [whitespace changes]

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

::: xpcom/base/nsMemoryReporterManager.cpp
@@ +499,5 @@
> +    NS_ENSURE_SUCCESS(rv, rv);
> +    return MOZ_COLLECT_REPORT(
> +      "vsize-max-contiguous", KIND_OTHER, UNITS_BYTES, amount,
> +      "Size of the maximum contiguous block of available virtual "
> +      "memory.");

There are some inconsistencies of whitespace around continuing lines that I'd like to call out. The style guide is not clear on whether one or the other style is preferred in this case, but:

Here it's """
  return MOZ_COLLECT_REPORT(
    arg1, arg2, ...
    argM, argN);

Above it's:
  if (!GetProcessMemoryInfo(GetCurrentProcess(),
                            (PROCESS_MEMORY_COUNTERS)...))

So in one case we're aligning with the opening paren and in the other case we're aligning to an indent level. It's not clear why to chosen these two different styles for each case. I suspect it's because in the GetProcessMemoryInfo case we're already nested inside parens and so choosing the correct indent level would be difficult. But I wish we could have a mechanical rule to apply here.

Comment 8

[Benjamin Smedberg]

Comment on attachment 8375284 [details] [diff] [review]
Fix style of xpcom/base/nsMemoryReporterManager.{h,cpp} [other changes]

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

r=me with nits addressed. If there are style issues that we should clarify in dev.platform feel free to punt on them for now.

::: xpcom/base/nsMemoryReporterManager.cpp
@@ +1321,1 @@
>  public:

Hrm, this didn't come up in the style guide, and we're not consistent about it in the tree. Typically for classes that multiply-inherit, this is formatted like constructor initializers:

class Int64Wrapper MOZ_FINAL
  : public nsISupports
  , public OtherBase

I wonder if we should recommend this style consistently even for classes with one or two bases.

::: xpcom/base/nsMemoryReporterManager.h
@@ +147,4 @@
>      mozilla::JSSizeOfTabFn    mJS;
>      mozilla::NonJSSizeOfTabFn mNonJS;
>  
>      SizeOfTabFns() { mozilla::PodZero(this); }

I believe we decided that inline methods should always be on following lines, even if they are trivial:

SizeOfTabFns()
{
  mozilla::PodZero(this);
}

@@ +199,1 @@
>      {}

Please put the closing brace of the empty method on the next line.

Comment 9

[Nicholas Nethercote [inactive]]

> How did you do the hand-done patch? Entirely by hand, or by using emacs
> indent-region, or something else?

Both patches were entirely by hand, but with some automated guidance. It was a while ago now, but IIRC I used some regular expressions to look for certain cases:

- /^{\n    / searches for blocks that start in column 1 and are immediately 4-indented, and variations on that find other cases.

- /\<while\>[^{]*$/ finds |while| blocks that lack opening braces.

Maybe some others. What I should do is find a few more files to fix up, and document better how I did them.

Vim's << command lets you shift blocks easily, so I didn't fix indenting with the delete key :)


> Typically for classes that multiply-inherit, this is
> formatted like constructor initializers:
> 
> class Int64Wrapper MOZ_FINAL
>   : public nsISupports
>   , public OtherBase
> 
> I wonder if we should recommend this style consistently even for classes
> with one or two bases.

Off the top of my head, I'd suggest requiring if there is more than one class, but allow a single base class to be on the same line. Not a strong opinion, though.

> There are some inconsistencies of whitespace around continuing lines

Yes. I'm not convinced it's a problem though -- this is one of those areas where a single style doesn't always make sense.
E.g. this style is common:

   foobar(abc,
          def,
          ghi);

But occasionally the function name is long enough that something else is better:

   mozilla::namespace::blah::ReallyLongMethodNameThatIsLong(
     longArgumentName1, longArgumentName2, longArgumentName3,
     longArgumentName4);

clang-format always went with a single style for these cases, and it caused a lot of low-value churn.

My take is that we reaching 100% style consistency is a non-goal. The real wins come from (a) not having to think about, or modify your habits, on the common items: indentation level, bracing of one-line blocks, where to put the '*' in pointer types, things like that; and (b) not having to conform to local style when modifying a file. So getting a file into 95% standard style is probably good enough to meet those criteria.

Finally, I really recommend doing a file or two by hand yourself to see which rules are important, which are less so, and which things surprise you.

Comment 10

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dc542bb8011c

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/dc542bb8011c

Comment 12

[Benjamin Smedberg]

> Off the top of my head, I'd suggest requiring if there is more than one
> class, but allow a single base class to be on the same line. Not a strong
> opinion, though.

We'll adopt that in the style guide for now.

> My take is that we reaching 100% style consistency is a non-goal.

My primary goal is having a useful automatic linter for existing code and incoming patches/PRs, both to make it easier for new people to contribute and to reduce the amount of time mentors and reviewers have to spend on these things.

This probably doesn't require 100% consistency: especially if the linter is smart enough to accept multiple correct indentations for some cases, we can let visual sense rule the cases like function continuations.

Comment 13

[Nicholas Nethercote [inactive]]

> My primary goal is having a useful automatic linter for existing code and
> incoming patches/PRs, both to make it easier for new people to contribute
> and to reduce the amount of time mentors and reviewers have to spend on
> these things.

That would be great, though not easy. Are you imagining a simple thing that just uses regexps or something involving a full-blown parser? IIRC, sdwilsh had a script for checking storage/ code (probably based on regexps) though I can't find it now.

Comment 14

[Nicholas Nethercote [inactive]]

Oh, and I wrote config/check_spidermonkey_style.py, which is a regexp-y thing, but it currently focuses almost entirely on checking that #include statements are in the correct form. This is one area where the SpiderMonkey style guide is *much* stricter and clearer than the Gecko style guide. The primary motivation was to ensure recursive #include cycles never happen (at one point SM had them, and it took a lot of work to remove them). |make check| will actually fail if the style checker fails.

One thing I learned from that is that you need to have flexibility in certain cases, such as imported third-party code, and for a regexp-y thing there are typically some tricky cases where it's hard to get things 100% right.