[clang-format] Add a test to check at once all the Mozilla coding style

[sylvestre.ledru]

Summary

To avoid potential regressions and checking most of the coding style aspects at once

[krasimir]

test/Format/check-coding-style-mozilla.cpp

10 ↗

(On Diff #88937)

What is tested here? Brace styles?

48 ↗

(On Diff #88937)

This does not check precisely what the comment says, because the comment affects the indentation decisions. Better put the comment before the class declaration.

75 ↗

(On Diff #88937)

I don't get it - shouldn't then TinyFunction be on a single line? Also the long trailing comment might affect formatting, so I suggest putting it before the function definition.

90 ↗

(On Diff #88937)

Trailing comments affect line breaking, so this is not really testing what the comments say. Suggest to put them on a line before the template.

102 ↗

(On Diff #88937)

I suggest either the comment to be more specific what exactly is "GOOD" or remove the comment altogether.

106 ↗

(On Diff #88937)

What does this fragment and the following ones test?

[sylvestre.ledru]

test/Format/check-coding-style-mozilla.cpp

10 ↗

(On Diff #88937)

Yes, do you want me to add a comment to explicit that?

48 ↗

(On Diff #88937)

I know, this is one of the thing I would like to see fixed in clang format or us.
I am adding it in the test suite to make sure that we address it

75 ↗

(On Diff #88937)

Same as above

90 ↗

(On Diff #88937)

Yeah, we are trying to fix this issue.
but you are correct, I will move it

106 ↗

(On Diff #88937)

Testing some issues which were reported. I added comments and removed some

[klimek]

test/Format/check-coding-style-mozilla.cpp

7–9 ↗

(On Diff #89022)

Note that I'm not a license expert, but I'd be surprised if it was ok to put code in random licenses into the repo.

[krasimir]

test/Format/check-coding-style-mozilla.cpp

10 ↗

(On Diff #88937)

I think that after the comments on the non-brace-style features below, it's obvious enough and doesn't need more comments.

48 ↗

(On Diff #88937)

I think this might be better addressed through a bug/feature request, plus an explicit comment here that this is not yet supported, because this is not obvious from just starring at the code.

[sylvestre.ledru]

Sorry, I tried to rename the file but this is too confusing for Phabricator it seems...

test/Format/check-coding-style-mozilla.cpp

7–9 ↗

(On Diff #89022)

I don't think this is a problem as it is in the test and there is no actual code but I can remove it if you prefer.

This was to test the formatting of comment

48 ↗

(On Diff #88937)

Make sense. I reported https://bugs.llvm.org/show_bug.cgi?id=32017 for this

90 ↗

(On Diff #88937)

Reported here:
https://bugs.llvm.org/show_bug.cgi?id=32016

[sylvestre.ledru]

@krasimir is that ok with you?

[krasimir]

In D30111#688379, @sylvestre.ledru wrote:

@krasimir is that ok with you?

Thank you for addressing my comments! The descriptions help to see what's supposed to happen.

Now another point with this test is that it tests that already Mozilla-valid source is not changed.
Maybe this is the right approach for a full style sanity test, I don't know.

unittests/Format/check-coding-style-mozilla.cpp

77 ↗

(On Diff #89094)

Note that here the trailing comment will make a version of TinyFunction on a single line exceed the line limit.
If the comment is put before the definition of TinyFunction, it could really be made tiny, as in:

// Tiny functions can be written in a single line.
int TinyFunction() { return mVar; }

[sylvestre.ledru]

I will rename it before the upload if that is fine with you.

unittests/Format/check-coding-style-mozilla.cpp

77 ↗

(On Diff #89094)

I will remove it for now. I will add it once we have the capability in clang-format

[sylvestre.ledru]

Manuel, is that ok with you? thanks (I will rename the file)

[krasimir]

My input:
Maybe this CL is trying to solve a non-existing problem, as separate issues may be tracked as bugs, fixed, and unittests added for each of them as appropriate, as happens in D30487, which adds an option to break multiple inheritance declarations,
but I'm not in position to judge that because I don't understand the philosophy of clang-format well enough.

[sylvestre.ledru]

This isn't about testing individual issues. This is to make sure that the Mozilla coding style remains consistent and we don't regress specific coding style. I found that easy and simple to have a single file to check for the specificities.

After https://reviews.llvm.org/D30487 is landed, I am planning to update this test.

[djasper]

Please don't add this as is. I don't usually run the file-based tests in my development workflow and suspect that I might be breaking this a lot.

If you want something like this, please add it as unittest(s) in unittests/Format/... (either in a new file or in an existing one)

Reasons:

• They are much faster to run.
• If a specific thing breaks, I find that much easier to narrow down and understand.

What problem is this addressing, though? Have we frequently changed formatting in a way that has negatively impacted Mozilla code?

[sylvestre.ledru]

What problem is this addressing, though? Have we frequently changed formatting in a way that has negatively impacted Mozilla code?

We are integrating more clang-format into gecko processes.
About the reason, I believed I explained them in the comment above https://reviews.llvm.org/D30111#690529
(tldr: I would like to have a file single file with all the Mozilla specificity)

[djasper]

So, while it might be convenient to view this all in one file, a test here is not convenient for me (or presumably other clang-format developers) to work with. You can make a pretty much 1:1 copy of it using a raw string literal in unittests. However, I don't think this is actually a good idea.

Over the years of working on clang-format we have discovered that you really need unittests and try to make them as specific about what you are trying to test as possible. There are many things in you patch, where you test really complex expressions that:

• I can envision arbitrary other changes to formatting that you wouldn't care about.
• Are much harder to debug for the specific issue than necessary, i.e. if something breaks, I'd have to reduce the test case further.

In general, I think this test adds very little on top of unittests we have for each single thing you configure in Mozilla style. I think the value added is that the combination of style flags used for Mozilla does work. But for that, really create a unittest that tests all the aspects of Mozilla style you care about with individual verifyFormat.. statements.

unittests/Format/CheckCodingStyleMozilla.cpp

+// RUN: clang-format -style=Mozilla %s > %T/foo.cpp 2>&1
+// RUN: diff -u %s %T/foo.cpp
+// RUN: rm -rf %T/foo.cpp
+
+/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
+/* vim: set ts=8 sts=2 et sw=2 tw=80: */
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+if (true) {
+} else if (false) {
+} else {
+}
+
+while (true) {
+}
+
+do {
+} while (true);
+
+for (auto bar::in) {
+}
+
+switch (var) {
+  case 1: {
+    // When you need to declare a variable in a switch, put the block in braces
+    int var;
+    break;
+  }
+  case 2:
+    foo();
+    break;
+  default:
+    break;
+}
+
+namespace mozilla {
+
+class MyClass : public A
+{
+  void Myclass();
+};
+
+// Not supported yet
+// See https://bugs.llvm.org/show_bug.cgi?id=32017 for the feature request
+class MyClass : public X // When deriving from more than one class, put each on
+                         // its own line.
+                ,
+                public Y
+{
+public:
+  MyClass(int aVar, int aVar2)
+    : mVar(aVar)
+    , mVar2(aVar2)
+  {
+    foo();
+  }
+
+  // Tiny constructors and destructors can be written on a single line.
+  MyClass() {}
+
+  // Unless it's a copy or move constructor or you have a specific reason to
+  // allow implicit conversions, mark all single-argument constructors explicit.
+  explicit MyClass(OtherClass aArg) { bar(); }
+
+  // This constructor can also take a single argument, so it also needs to be
+  // marked explicit.
+  explicit MyClass(OtherClass aArg, AnotherClass aArg2 = AnotherClass())
+  {
+    foo();
+  }
+
+  int TinyFunction()
+  {
+    return mVar;
+  } // Tiny functions can be written in a single line.
+
+  int LargerFunction()
+  {
+    bar();
+    foo();
+  }
+
+private:
+  int mVar;
+};
+
+} // namespace mozilla
+
+template<typename T> // Templates on own line.
+static int
+MyFunction(int a) // Return type on own line for top-level functions.
+{
+  foo();
+}
+
+int
+MyClass::Method(long b)
+{
+  bar();
+}
+
+T* p; // Correct declaration style
+
+// Test the && and || with parentheses
+return (nextKeyframe < aTimeThreshold ||
+        (mVideo.mTimeThreshold &&
+         mVideo.mTimeThreshold.ref().EndTime() < aTimeThreshold)) &&
+       nextKeyframe.ToMicroseconds() >= 0 && !nextKeyframe.IsInfinite();
+
+// The ? should be placed 2 spaces after the previous declaration
+LOGV("%d audio samples demuxed (sid:%d)",
+     aSamples->mSamples.Length(),
+     aSamples->mSamples[0]->mTrackInfo
+       ? aSamples->mSamples[0]->mTrackInfo->GetID()
+       : 0);
+
+// Test with the 80 chars limit
+return (aDecoder.HasPromise() || aDecoder.mTimeThreshold.isSome()) &&
+       !aDecoder.HasPendingDrain() && !aDecoder.HasFatalError() &&
+       !aDecoder.mDemuxRequest.Exists() && !aDecoder.mOutput.Length() &&
+       !aDecoder.HasInternalSeekPending() && !aDecoder.mDecodeRequest.Exists();
+
+template<int>
+void
+foo();
+
+#define MACRO(V)                                                               \
+  V(Rt2) /* one more char */                                                   \
+  V(Rs)  /* than here  */                                                      \
+/* comment 3 */