Page MenuHomePhabricator

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

Authored by sylvestre.ledru on Feb 17 2017, 11:52 AM.

Details

Reviewers
klimek
djasper
Summary

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

Diff Detail

Event Timeline

krasimir added inline comments.
test/Format/check-coding-style-mozilla.cpp
11

What is tested here? Brace styles?

49

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

76

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.

91

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.

103

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

107

What does this fragment and the following ones test?

sylvestre.ledru marked 3 inline comments as done.
sylvestre.ledru added inline comments.
test/Format/check-coding-style-mozilla.cpp
11

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

49

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

76

Same as above

91

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

107

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

klimek added inline comments.Feb 20 2017, 1:45 AM
test/Format/check-coding-style-mozilla.cpp
8–10

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 added inline comments.Feb 20 2017, 1:56 AM
test/Format/check-coding-style-mozilla.cpp
11

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

49

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 updated this revision to Diff 89094.
sylvestre.ledru marked 3 inline comments as done.

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

test/Format/check-coding-style-mozilla.cpp
8–10

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

49

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

91

@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; }

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

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

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.

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 added a subscriber: djasper.Mar 2 2017, 7:58 AM

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?

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 requested changes to this revision.Mar 2 2017, 10:29 PM

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.

This revision now requires changes to proceed.Mar 2 2017, 10:29 PM
sylvestre.ledru abandoned this revision.Sep 17 2018, 12:06 AM