1508281 - See if we need to turn off clang-format anywhere in toolkit/components/telemetry

Status: RESOLVED FIXED

(Toolkit :: Telemetry, enhancement, P1)

Description

[Chris H-C :chutten]

As mentioned on dcamp-all the style for whitespace is changing and will be enforced using clang-format. We should see what this looks like for our C++ code and if we need to code some exemptions.

Of particular note may be our generated source files. They shouldn't be checked, but we should verify/ask Ehsan about it.

Comment 1

[(no longer active)]

Generated source files shouldn't be checked, no.  If they are, that's a bug.  :-)

Comment 2

[Chris H-C :chutten]

This repo has clang-format applied https://github.com/sylvestre/gecko-dev/blob/master/toolkit/components/telemetry/core/

Comment 3

[Jan-Erik Rediger [:janerik]]

I've gone through core/ipc, geckoview/, other/ and tests/gtest so far.
Some notable things:

* `private:`, `public:`, etc. are indented by one space, which feels odd.
* the 80-character limit is causing quite a bit of churn (I would have preferred 100 chars, but consistency over personal preference!)
* a lot of actual indentation fixes and repositioning of `&` and `*` (close to the type, not the name) (Yey!)


Given that we can also use clang-format in the future to keep it all that way, nothing of that blocks the reformatting.

Next: our core code.

Comment 4

[Jan-Erik Rediger [:janerik]]

One more thing: our mode lines are now broken into 2 lines:

this:

  /* -*-  Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2; -*- */ 

becomes this:

  /* -*-  Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2; -*-
   */

I haven't checked myself, but seen it elsewhere in the code that it wasn't broken off when the line started with `/**` (two stars). (Switching that everywhere now is too much effort IMO for little gain.)

Comment 5

[Jan-Erik Rediger [:janerik]]

For core/:

* Some nicely manually aligned things are now (auto-)unaligned. IMO that is fine and not too distracting.
* sorted `using` lines. Hooray!
* I still don't like that the 80-char limit pushed a lot of our log messages to a new line


Some things I am not happy about:

1. This is weird formatting (https://github.com/sylvestre/gecko-dev/commit/b071d70cefcbf29193b1ad945dfb27ce503988a4#diff-22609026e88184f2cd2fd52da58a8d45R1796):

    TelemetryScalar::Add(mozilla::Telemetry::ScalarID::
                             TELEMETRY_ACCUMULATE_UNKNOWN_HISTOGRAM_KEYS,
                         NS_ConvertASCIItoUTF16(gHistogramInfos[id].name()), 1);


2. Our nicely manually formatted huge static_assert got messed up as well:
https://github.com/sylvestre/gecko-dev/commit/b071d70cefcbf29193b1ad945dfb27ce503988a4#diff-22609026e88184f2cd2fd52da58a8d45R1997

3. Doc strings over multiple lines have their leading whitespace removed (only including one example here, there are more):
https://github.com/sylvestre/gecko-dev/commit/b071d70cefcbf29193b1ad945dfb27ce503988a4#diff-f0ef017ddb34a0abd2ef6bf4e0592409R1483

4. Our example of the returned JSON got split across lines:
https://github.com/sylvestre/gecko-dev/commit/b071d70cefcbf29193b1ad945dfb27ce503988a4#diff-f0ef017ddb34a0abd2ef6bf4e0592409R2853

5. Our example of the returned JSON got concatenated to the previous line (because no empty line in between?):
https://github.com/sylvestre/gecko-dev/commit/b071d70cefcbf29193b1ad945dfb27ce503988a4#diff-f0ef017ddb34a0abd2ef6bf4e0592409R2940

Comment 6

[Jan-Erik Rediger [:janerik]]

Nothing of the above (or anything I didn't mention) changes any semantics and IMO it's mostly an improvement.

Should we do anything about the above?

Comment 7

[(no longer active)]

One note about mode-lines and mis-formatted comments: I'm spending time in bug 1508472 to go across the entire tree and do a best effort attempt at preserving as much of the comment formatting as I humanly can.  :-)  (I have yet to get to toolkit/).  If you'd like to assist with the telemetry part, that would be a huge help!

Also about cases where clang-format misformats things like carefully lined up static_assert's and what not, you should feel free to use // clang-format off/on blocks around such code to preserve the formatting where it makes sense.  If you use dxr/searchfox to search for existing occurrences you can find a lot of examples already.  (I'd be happy to help review such patches if needed!)

Comment 8

[Jan-Erik Rediger [:janerik]]

4 + 5 can be ignored, those lines are actually gone in m-c (documentation resides in nsITelemetry.idl)
I will add the clang-format off for the static_assert (number 2 from above list).

That leaves 1 and 3.

Comment 9

[Chris H-C :chutten]

(In reply to Jan-Erik Rediger [:janerik] from comment #5)
> For core/:
> 
> * Some nicely manually aligned things are now (auto-)unaligned. IMO that is
> fine and not too distracting.
> * sorted `using` lines. Hooray!
> * I still don't like that the 80-char limit pushed a lot of our log messages
> to a new line
> 
> 
> Some things I am not happy about:
> 
> 1. This is weird formatting
> (https://github.com/sylvestre/gecko-dev/commit/
> b071d70cefcbf29193b1ad945dfb27ce503988a4#diff-
> 22609026e88184f2cd2fd52da58a8d45R1796):
> 
>     TelemetryScalar::Add(mozilla::Telemetry::ScalarID::
>                              TELEMETRY_ACCUMULATE_UNKNOWN_HISTOGRAM_KEYS,
>                          NS_ConvertASCIItoUTF16(gHistogramInfos[id].name()),
> 1);

My biggest problem with this one is that it indents by four spaces instead of two.

I wonder if there are many conforming formats here... whether we could adjust the formatting ourselves ahead of time to avoid this.

> 3. Doc strings over multiple lines have their leading whitespace removed
> (only including one example here, there are more):
> https://github.com/sylvestre/gecko-dev/commit/
> b071d70cefcbf29193b1ad945dfb27ce503988a4#diff-
> f0ef017ddb34a0abd2ef6bf4e0592409R1483

Ugh. Ehsan, any hints/tips/tricks for this?

Comment 10

[(no longer active)]

(In reply to Chris H-C :chutten from comment #9)
> > 3. Doc strings over multiple lines have their leading whitespace removed
> > (only including one example here, there are more):
> > https://github.com/sylvestre/gecko-dev/commit/
> > b071d70cefcbf29193b1ad945dfb27ce503988a4#diff-
> > f0ef017ddb34a0abd2ef6bf4e0592409R1483
> 
> Ugh. Ehsan, any hints/tips/tricks for this?

I preemptively answered in comment 7.  Does that help?

Comment 11

[Chris H-C :chutten]

I didn't quite understand what part of comment 7 would help us here. Waiting for you to slog through the rest of comment-land seemed like the least scalable answer :)

Comment 12

[(no longer active)]

(In reply to Chris H-C :chutten from comment #11)
> I didn't quite understand what part of comment 7 would help us here. Waiting
> for you to slog through the rest of comment-land seemed like the least
> scalable answer :)

To rephrase, what I was trying to say was: there is no automated tooling here that can automatically format arbitrary comments including various ASCII art and whitespace alignments.  If you don't do anything in particular here I will get to the occurrences of issues like number 3 in bug 1508472 (not because that's a scalable way to solve this problem, but because going through a ~4MB diff is a soul crushing task that nobody else was really in a rush to volunteer to do!).  I'm trying to do my best to ensure that I catch all of the instances of issues like number 3 in that bug (you can have a look at the patches that have landed there already.)

In case you'd be willing to lend a hand and help going through toolkit/components/telemetry and take care of misformatted comments similar to how I've been doing that there, I would highly appreciate that.  If you'd like to do that, you can download attachment 9027674 [details] and find the changes to toolkit/components/telemetry there and see what comments have been misformatted in that diff and fix them on m-c before Friday.  In that case, please do let me know so that we don't duplicate work.  :-)  (If you need help please ping me on IRC/slack.)  Thanks!

Comment 13

[(no longer active)]

OK, I'm now starting to look at the toolkit/components/telemetry portions of the comments diff in bug 1508472.  :-)  I'll post the results on the bug in a few hours.

Comment 14

[Jan-Erik Rediger [:janerik]]

Attachment: Bug 1508281 - Don't reformat long static_assert

Comment 15

[Alessio Placitelli [:Dexter]]

(In reply to Jan-Erik Rediger [:janerik] from comment #6)
> Should we do anything about the above?

I'd vote for disabling messing with the static_asserts, as you suggested :)

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/15fb49ca9d9f
Don't reformat long static_assert r=chutten

Comment 17

[Jan-Erik Rediger [:janerik]]

With the limited time remaining, I will land the static_assert fix only. If :ehsan was able to go through the comment parts even better, if not we should fix the then-misaligned comments after the merge.

Comment 18

[(no longer active)]

(In reply to Jan-Erik Rediger [:janerik] from comment #17)
> With the limited time remaining, I will land the static_assert fix only. If
> :ehsan was able to go through the comment parts even better, if not we
> should fix the then-misaligned comments after the merge.

I did, it's fixed as part of https://hg.mozilla.org/mozilla-central/rev/04f0bbf40bf3.

Comment 19

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/15fb49ca9d9f