[PATCH] D30111: [clang-format] Add a test to check at once all the Mozilla coding style
djasper requested changes to this revision. djasper added a comment. This revision now requires changes to proceed. 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. https://reviews.llvm.org/D30111 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30111: [clang-format] Add a test to check at once all the Mozilla coding style
sylvestre.ledru added a comment. > 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) https://reviews.llvm.org/D30111 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30111: [clang-format] Add a test to check at once all the Mozilla coding style
djasper added a comment. 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? https://reviews.llvm.org/D30111 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30111: [clang-format] Add a test to check at once all the Mozilla coding style
sylvestre.ledru added a comment. 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. https://reviews.llvm.org/D30111 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30111: [clang-format] Add a test to check at once all the Mozilla coding style
krasimir added a comment. 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 https://reviews.llvm.org/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. https://reviews.llvm.org/D30111 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30111: [clang-format] Add a test to check at once all the Mozilla coding style
sylvestre.ledru added a comment. Manuel, is that ok with you? thanks (I will rename the file) https://reviews.llvm.org/D30111 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30111: [clang-format] Add a test to check at once all the Mozilla coding style
sylvestre.ledru added a comment.
I will rename it before the upload if that is fine with you.
Comment at: unittests/Format/check-coding-style-mozilla.cpp:77
+return mVar;
+ } // Tiny functions can be written in a single line.
+
krasimir wrote:
> 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; }
> ```
I will remove it for now. I will add it once we have the capability in
clang-format
https://reviews.llvm.org/D30111
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30111: [clang-format] Add a test to check at once all the Mozilla coding style
sylvestre.ledru updated this revision to Diff 90053.
Herald added a subscriber: mgorny.
https://reviews.llvm.org/D30111
Files:
test/Format/check-coding-style-mozilla.cpp
unittests/Format/CMakeLists.txt
Index: unittests/Format/CMakeLists.txt
===
--- unittests/Format/CMakeLists.txt
+++ unittests/Format/CMakeLists.txt
@@ -14,6 +14,7 @@
NamespaceEndCommentsFixerTest.cpp
SortImportsTestJS.cpp
SortIncludesTest.cpp
+ check-coding-style-mozilla.cpp
)
target_link_libraries(FormatTests
Index: test/Format/check-coding-style-mozilla.cpp
===
--- test/Format/check-coding-style-mozilla.cpp
+++ test/Format/check-coding-style-mozilla.cpp
@@ -0,0 +1,130 @@
+// 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();
+};
+
+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 // 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
+void
+foo();
+
+#define MACRO(V) \
+ V(Rt2) /* one more char */ \
+ V(Rs) /* than here */ \
+/* comment 3 */
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30111: [clang-format] Add a test to check at once all the Mozilla coding style
krasimir added a comment.
In https://reviews.llvm.org/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.
Comment at: unittests/Format/check-coding-style-mozilla.cpp:77
+return mVar;
+ } // Tiny functions can be written in a single line.
+
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; }
```
https://reviews.llvm.org/D30111
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30111: [clang-format] Add a test to check at once all the Mozilla coding style
sylvestre.ledru added a comment. @krasimir is that ok with you? https://reviews.llvm.org/D30111 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30111: [clang-format] Add a test to check at once all the Mozilla coding style
sylvestre.ledru updated this revision to Diff 89094.
sylvestre.ledru added a comment.
Sorry, I tried to rename the file but this is too confusing for Phabricator it
seems...
https://reviews.llvm.org/D30111
Files:
unittests/Format/check-coding-style-mozilla.cpp
Index: unittests/Format/check-coding-style-mozilla.cpp
===
--- unittests/Format/check-coding-style-mozilla.cpp
+++ unittests/Format/check-coding-style-mozilla.cpp
@@ -0,0 +1,132 @@
+// 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 // 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
+void
+foo();
+
+#define MACRO(V) \
+ V(Rt2) /* one more char */ \
+ V(Rs) /* than here */ \
+/* comment 3 */
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30111: [clang-format] Add a test to check at once all the Mozilla coding style
sylvestre.ledru added inline comments.
Comment at: test/Format/check-coding-style-mozilla.cpp:48
+,
+public Y
+{
krasimir wrote:
> sylvestre.ledru wrote:
> > krasimir wrote:
> > > This does not check precisely what the comment says, because the comment
> > > affects the indentation decisions. Better put the comment before the
> > > class declaration.
> > 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
> 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.
Make sense. I reported https://bugs.llvm.org/show_bug.cgi?id=32017 for this
Comment at: test/Format/check-coding-style-mozilla.cpp:90
+template // Templates on own line.
+static int // Return type on own line for top-level functions.
+ MyFunction(int a)
sylvestre.ledru wrote:
> krasimir wrote:
> > 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.
> Yeah, we are trying to fix this issue.
> but you are correct, I will move it
Reported here:
https://bugs.llvm.org/show_bug.cgi?id=32016
Comment at: test/Format/check-coding-style-mozilla.cpp:7-9
+/* 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/. */
klimek wrote:
> 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.
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
https://reviews.llvm.org/D30111
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30111: [clang-format] Add a test to check at once all the Mozilla coding style
sylvestre.ledru updated this revision to Diff 89093.
sylvestre.ledru marked 3 inline comments as done.
https://reviews.llvm.org/D30111
Files:
unittests/Format/CheckCodingStyleMozilla.cpp
Index: unittests/Format/CheckCodingStyleMozilla.cpp
===
--- unittests/Format/CheckCodingStyleMozilla.cpp
+++ unittests/Format/CheckCodingStyleMozilla.cpp
@@ -0,0 +1,132 @@
+// 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 // 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
+void
+foo();
+
+#define MACRO(V) \
+ V(Rt2) /* one more char */ \
+ V(Rs) /* than here */ \
+/* comment 3 */
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30111: [clang-format] Add a test to check at once all the Mozilla coding style
krasimir added inline comments.
Comment at: test/Format/check-coding-style-mozilla.cpp:10
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+if (true) {
sylvestre.ledru wrote:
> krasimir wrote:
> > What is tested here? Brace styles?
> Yes, do you want me to add a comment to explicit that?
I think that after the comments on the non-brace-style features below, it's
obvious enough and doesn't need more comments.
Comment at: test/Format/check-coding-style-mozilla.cpp:48
+,
+public Y
+{
sylvestre.ledru wrote:
> krasimir wrote:
> > This does not check precisely what the comment says, because the comment
> > affects the indentation decisions. Better put the comment before the class
> > declaration.
> 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
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.
https://reviews.llvm.org/D30111
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30111: [clang-format] Add a test to check at once all the Mozilla coding style
klimek added inline comments. Comment at: test/Format/check-coding-style-mozilla.cpp:7-9 +/* 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/. */ 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. https://reviews.llvm.org/D30111 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30111: [clang-format] Add a test to check at once all the Mozilla coding style
sylvestre.ledru marked 3 inline comments as done.
sylvestre.ledru added inline comments.
Comment at: test/Format/check-coding-style-mozilla.cpp:10
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+if (true) {
krasimir wrote:
> What is tested here? Brace styles?
Yes, do you want me to add a comment to explicit that?
Comment at: test/Format/check-coding-style-mozilla.cpp:48
+,
+public Y
+{
krasimir wrote:
> This does not check precisely what the comment says, because the comment
> affects the indentation decisions. Better put the comment before the class
> declaration.
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
Comment at: test/Format/check-coding-style-mozilla.cpp:75
+return mVar;
+ } // Tiny functions can be written in a single line.
+
krasimir wrote:
> 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.
Same as above
Comment at: test/Format/check-coding-style-mozilla.cpp:90
+template // Templates on own line.
+static int // Return type on own line for top-level functions.
+ MyFunction(int a)
krasimir wrote:
> 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.
Yeah, we are trying to fix this issue.
but you are correct, I will move it
Comment at: test/Format/check-coding-style-mozilla.cpp:106
+ !GetCachedStyleData(aSID),
+ "bar");
+
krasimir wrote:
> What does this fragment and the following ones test?
Testing some issues which were reported. I added comments and removed some
https://reviews.llvm.org/D30111
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30111: [clang-format] Add a test to check at once all the Mozilla coding style
sylvestre.ledru updated this revision to Diff 89022.
https://reviews.llvm.org/D30111
Files:
test/Format/check-coding-style-mozilla.cpp
Index: test/Format/check-coding-style-mozilla.cpp
===
--- test/Format/check-coding-style-mozilla.cpp
+++ test/Format/check-coding-style-mozilla.cpp
@@ -0,0 +1,130 @@
+// 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();
+};
+
+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 // 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
+void
+foo();
+
+#define MACRO(V) \
+ V(Rt2) /* one more char */ \
+ V(Rs) /* than here */ \
+/* comment 3 */
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30111: [clang-format] Add a test to check at once all the Mozilla coding style
krasimir added inline comments.
Comment at: test/Format/check-coding-style-mozilla.cpp:10
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+if (true) {
What is tested here? Brace styles?
Comment at: test/Format/check-coding-style-mozilla.cpp:48
+,
+public Y
+{
This does not check precisely what the comment says, because the comment
affects the indentation decisions. Better put the comment before the class
declaration.
Comment at: test/Format/check-coding-style-mozilla.cpp:75
+return mVar;
+ } // Tiny functions can be written in a single line.
+
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.
Comment at: test/Format/check-coding-style-mozilla.cpp:90
+template // Templates on own line.
+static int // Return type on own line for top-level functions.
+ MyFunction(int a)
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.
Comment at: test/Format/check-coding-style-mozilla.cpp:102
+
+T* p; // GOOD
+
I suggest either the comment to be more specific what exactly is "GOOD" or
remove the comment altogether.
Comment at: test/Format/check-coding-style-mozilla.cpp:106
+ !GetCachedStyleData(aSID),
+ "bar");
+
What does this fragment and the following ones test?
https://reviews.llvm.org/D30111
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30111: [clang-format] Add a test to check at once all the Mozilla coding style
sylvestre.ledru created this revision.
To avoid potential regressions and checking most of the coding style aspects at
once
https://reviews.llvm.org/D30111
Files:
test/Format/check-coding-style-mozilla.cpp
Index: test/Format/check-coding-style-mozilla.cpp
===
--- test/Format/check-coding-style-mozilla.cpp
+++ test/Format/check-coding-style-mozilla.cpp
@@ -0,0 +1,136 @@
+// 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();
+};
+
+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 // Templates on own line.
+static int // Return type on own line for top-level functions.
+ MyFunction(int a)
+{
+ foo();
+}
+
+int
+MyClass::Method(long b)
+{
+ bar();
+}
+
+T* p; // GOOD
+
+MOZ_ASSERT(!mChild && !(mBits & nsCachedStyleData::GetBitForSID(aSID)) &&
+ !GetCachedStyleData(aSID),
+ "bar");
+
+return (nextKeyframe < aTimeThreshold ||
+(mVideo.mTimeThreshold &&
+ mVideo.mTimeThreshold.ref().EndTime() < aTimeThreshold)) &&
+ nextKeyframe.ToMicroseconds() >= 0 && !nextKeyframe.IsInfinite();
+
+LOGV("%d audio samples demuxed (sid:%d)",
+ aSamples->mSamples.Length(),
+ aSamples->mSamples[0]->mTrackInfo
+ ? aSamples->mSamples[0]->mTrackInfo->GetID()
+ : 0);
+
+return (aDecoder.HasPromise() || aDecoder.mTimeThreshold.isSome()) &&
+ !aDecoder.HasPendingDrain() && !aDecoder.HasFatalError() &&
+ !aDecoder.mDemuxRequest.Exists() && !aDecoder.mOutput.Length() &&
+ !aDecoder.HasInternalSeekPending() && !aDecoder.mDecodeRequest.Exists();
+
+if ((decoder.mWaitingForData &&
+ (!decoder.mTimeThreshold || decoder.mTimeThreshold.ref().mWaiting)) ||
+(decoder.mWaitingForKey && decoder.mDecodeRequest.Exists())) {
+}
+
+template
+void
+foo();
+
+#define MACRO(V) \
+ V(Rt2) /* one more char */ \
+ V(Rs) /* than here */ \
+/* comment 3 */
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
