966840 - Reformat whitespace using clang-format

Status: RESOLVED WONTFIX

(Core :: XPConnect, defect)

Description

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

~/.mozbuild/clang-format-3.5 --style="{BasedOnStyle: Mozilla, DerivePointerBinding: false}" -i *.cpp *.h

Comment 1

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Reformat XPConnect using clang-format;

Comment 2

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8369280 [details] [diff] [review]
Reformat XPConnect using clang-format;

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

::: js/xpconnect/src/BackstagePass.h
@@ +41,4 @@
>  };
>  
> +NS_EXPORT
> +nsresult

Is this really the style we want to have? It seems pointless and wasteful in terms of lines-on-screen-consumed to have NS_EXPORT and the return type on separate lines. I see in other files that "static" and the return type are kept on the same line.

::: js/xpconnect/src/Sandbox.cpp
@@ +239,5 @@
> +  return true;
> +}
> +
> +namespace xpc
> +{

brace should be on same line as "namespace xpc" according to the guide.

@@ +372,3 @@
>  }
>  
> +enum ForwarderCloneTags { SCTAG_BASE = JS_SCTAG_USER_MIN, SCTAG_REFLECTOR };

I think this is the wrong direction to go, though it isn't in the coding style guide. One line per enum literal seems better.

@@ +426,4 @@
>  }
>  
>  static const JSStructuredCloneCallbacks gForwarderStructuredCloneCallbacks = {
> +  CloneNonReflectorsRead, CloneNonReflectorsWrite, nullptr

Again, one item per line seems better.

@@ +786,4 @@
>  }
>  
> +template <typename Op>
> +bool

The style guide should be clarified as to which of these two is preferred:

    template <typename T>
    ReturnType
    FunctionName(...);

or:

    template <typename T> ReturnType
    FunctionName(...);

@@ +1521,4 @@
>  }
>  
>  // static
>  nsresult

Which is better:

    // static
    nsreuslt

or:

    /*static*/ nsresult

or:

    nsresult

Comment 3

[Bobby Holley (:bholley)]

Comment on attachment 8369280 [details] [diff] [review]
Reformat XPConnect using clang-format;

Cross-posted from dev-platform:

XPConnect currently follows JS-style, which is the most divergent style in the tree (in particular, 4-space indents mean that a restyle is going to rewrite every line). As such, I don't think it's a great place to prototype clang-format restyles. At minimum, we should do dom/ and content/ first.

Comment 4

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Reformat nsMemoryReporterManager using clang-format;

Comment 5

[Nicholas Nethercote [inactive]]

(In reply to Anthony Jones (:kentuckyfriedtakahe, :k17e) from comment #4)
> Created attachment 8369308 [details] [diff] [review]
> Reformat nsMemoryReporterManager using clang-format;

The patch is empty.

Comment 6

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Reformat nsMemoryReporterManager using clang-format;

Comment 7

[Seth Fowler [:seth] [:s2h]]

My two cents:

(In reply to Brian Smith (:briansmith, :bsmith; NEEDINFO? for response) from comment #2)
> The style guide should be clarified as to which of these two is preferred:
> 
>     template <typename T>
>     ReturnType
>     FunctionName(...);
> 
> or:
> 
>     template <typename T> ReturnType
>     FunctionName(...);

The former, I suspect, as the latter is going to get unreadable fast in complex template code with many/defaulted parameters.


> Which is better:
> 
>     // static
>     nsreuslt
> 
> or:
> 
>     /*static*/ nsresult
> 
> or:
> 
>     nsresult

/*static*/ nsresult is IMO the clear winner. The third option should not even better considered.

Comment 8

[Nicholas Nethercote [inactive]]

Differences between my hand-written fixes and the auto ones:

- clang-format doesn't allow class data members to be aligned like this:

    int    mInt;
    double mDouble;
    char   mChar;

  I think this kind of alignment is very common and should be allowed. This was my biggest gripe.

- It also modified a number of multi-line statements in ways that didn't seem any better than what we had.

- It spaced out NS_MEMORY_REPORTER_MANAGER_CID in a way that's quite ugly.

- It butchered some multi-line string literals.

- My patch also added braces to single-statement blocks, and converted some args to |aFoo| form.

Plus a few other minor differences.

Overall, it did a reasonably good job, but there are clearly lots of fixes that have to be done by hand as follow-ups.

Can you tell it to modify whitespace at the start of and perhaps the end of statements, but not in the middle? I think that would help a lot, and then I think it would be reasonable to recommend that style patches consist of clang-format changes plus follow-up hand-written changes.

Comment 9

[Nicholas Nethercote [inactive]]

Attachment: Diff between my hand-written style fixes and the clang-format ones

Comment 10

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Nicholas Nethercote [:njn] from comment #8)
> - clang-format doesn't allow class data members to be aligned like this:
> 
>     int    mInt;
>     double mDouble;
>     char   mChar;
> 
>   I think this kind of alignment is very common and should be allowed. This
> was my biggest gripe.

Probably worth discussing on dev-platform or somewhere. I think a lot of people would prefer 100% unaligned because then you don't have to realign things each time you add or remove one.

Comment 11

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

(In reply to Nicholas Nethercote [:njn] from comment #8)
> Differences between my hand-written fixes and the auto ones:
> 
> - clang-format doesn't allow class data members to be aligned like this:
> 
>     int    mInt;
>     double mDouble;
>     char   mChar;
> 
>   I think this kind of alignment is very common and should be allowed. This
> was my biggest gripe.

There isn't an option to support this alignment. It doesn't appear to be a rule in Mozilla so I would argue that it is probably more trouble than it is worth adding support for it.

> - It also modified a number of multi-line statements in ways that didn't
> seem any better than what we had.

It reformats everything.

> - It spaced out NS_MEMORY_REPORTER_MANAGER_CID in a way that's quite ugly.

I've added a Mozilla bracing style that includes a tweak for this. What you end up with makes some sense:

#define NS_MEMORY_REPORTER_MANAGER_CID                                         \
    { 0xfb97e4f5, 0x32dd, 0x497a,                                              \
      { 0xba, 0xa2, 0x7d, 0x1e, 0x55, 0x7, 0x99, 0x10 } }

> - It butchered some multi-line string literals.

It seems to split string literals and comments but not join them.

> Can you tell it to modify whitespace at the start of and perhaps the end of
> statements, but not in the middle? I think that would help a lot, and then I
> think it would be reasonable to recommend that style patches consist of
> clang-format changes plus follow-up hand-written changes.

There isn't an option for that. We'd be best to avoid hand-written changes except for changes that clang-format will preserve such as comments and string line breaks.

Comment 12

[Bobby Holley (:bholley)]

(In reply to Anthony Jones (:kentuckyfriedtakahe, :k17e) from comment #11)
> (In reply to Nicholas Nethercote [:njn] from comment #8)
> > Differences between my hand-written fixes and the auto ones:
> > 
> > - clang-format doesn't allow class data members to be aligned like this:
> > 
> >     int    mInt;
> >     double mDouble;
> >     char   mChar;
> > 
> >   I think this kind of alignment is very common and should be allowed. This
> > was my biggest gripe.
> 
> There isn't an option to support this alignment. It doesn't appear to be a
> rule in Mozilla so I would argue that it is probably more trouble than it is
> worth adding support for it.

I think this is a style guide question, and whether or not clang-format currently supports it should not dictate the decision.
> 
> > - It also modified a number of multi-line statements in ways that didn't
> > seem any better than what we had.
> 
> It reformats everything.

Hm. I think we really want to avoid the situation where clang-format rewrites something for no apparent reason. We should aim for a state where a developer who knows the style guide should be able to hand-produce a patch that produces zero nits from clang-format.

> 
> > - It spaced out NS_MEMORY_REPORTER_MANAGER_CID in a way that's quite ugly.
> 
> I've added a Mozilla bracing style that includes a tweak for this. What you
> end up with makes some sense:

That seems 1-indent too much.

> 
> > - It butchered some multi-line string literals.
> 
> It seems to split string literals and comments but not join them.

Seems like we need to fix that.

>
> > Can you tell it to modify whitespace at the start of and perhaps the end of
> > statements, but not in the middle? I think that would help a lot, and then I
> > think it would be reasonable to recommend that style patches consist of
> > clang-format changes plus follow-up hand-written changes.
> 
> There isn't an option for that.

Again, I don't think the current capabilities of clang-format should dictate our style. I'm not necessarily agreeing with the feature request here, but just saying that we should catalog the gripes, and come to a consensus on what fixes we need in clang-format for it to become acceptable.

Comment 13

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

(In reply to Bobby Holley (:bholley) from comment #12)
> I think this is a style guide question, and whether or not clang-format
> currently supports it should not dictate the decision.

Given that the current style is not specified and the de-facto appears to be unaligned I don't see why not.

Comment 14

[Bobby Holley (:bholley)]

(In reply to Anthony Jones (:kentuckyfriedtakahe, :k17e) from comment #13)
> Given that the current style is not specified and the de-facto appears to be
> unaligned I don't see why not.

I'm not convinced it's a de-facto standard. It should go on a list of things that we run by bsmedberg.

Comment 15

[Nicholas Nethercote [inactive]]

> We'd be best to avoid hand-written changes
> except for changes that clang-format will preserve such as comments and
> string line breaks.

That's not realistic. There are tons of style fixes that clang-format cannot do, so any comprehensive style fix will unavoidably involve follow-up fixes by hand.

The most important whitespace is that at the start of the line. I'm uncomfortable with a tool that changes whitespace other than that, because the test has shown there's lots of different possibilities and it'll end up modifying lots of lines for no benefit... things like converting this:

  foo(one, two, three, four,
      five, six, seven, eight)

to this:

  foo(one, two, three, four, five,
      six, seven, eight)

So IMO the ideal workflow is:
- use a conservative tool that fixes the unambiguously wrong things
- fix everything else by hand

I'm not sure if that tool exists, though.

Finally, I think you're underestimating how often extra whitespace is used within lines to achieve vertical alignment.