Don't allow short if/case/blocks on a single line #2044
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
While looking at code changes after mass clang-formatting our current codebase, I think this rather hurts readability than improves it. And none of preset styles (LLVM, Chrome, Google, Mozilla, and WebKit) allows these, with only exception that Google allows short ifs on a single line.
kripken
approved these changes
Apr 23, 2019
aheejin
added a commit
to aheejin/binaryen
that referenced
this pull request
Apr 24, 2019
This applies clang-format to the whole codebase. Below are manual tweaks
I had to make:
1. After clang-format changed the order of #includes, the code failed to
compile, so this also adds some missing header guards and more #includes
to make code compile.
2. After applying clang-format to this kind of code,
```
cashew::IString GLOBAL("global"),
NAN_("NaN"),
INFINITY_("Infinity"),
NAN__("nan"),
INFINITY__("infinity"),
TOPMOST("topmost"),
INT8ARRAY("Int8Array"),
...
```
This becomes
```
cashew::IString GLOBAL("global"), NAN_("NaN"), INFINITY_("Infinity"),
NAN__("nan"), INFINITY__("infinity"), TOPMOST("topmost"),
INT8ARRAY("Int8Array"), INT16ARRAY("Int16Array"),
...
```
I fixed the formatting by adding the type to every line as follows:
```
cashew::IString GLOBAL("global");
cashew::IString NAN_("NaN");
cashew::IString INFINITY_("Infinity");
cashew::IString NAN__("nan");
cashew::IString INFINITY__("infinity");
cashew::IString TOPMOST("topmost");
cashew::IString INT8ARRAY("Int8Array");
...
```
3. I fixed many parts of the comments for readability. The most
frequently occurring example was this:
```
SomeClass var = 123; // very very very very very very very very very very very very very very very very long comment
```
clang-format blinds transforms this into:
```
Someclass var
= 123; // very very very very very very very very very very very very
// very very very very long comment
```
I fixed these to:
```
// very very very very very very very very very very very very very very
// very very long comment
SomeClass var = 123;
```
4. I ended up using `clang-format off` and `clang-format on` to
explicitly disable clang-format only in one place in
`src/support/safe_integer.cpp`. We couldn't maintain its readability of
the last comment block there.
---
Please let me know if there are any suggestions to this or our current
.clang-format configuration. I tweaked our clang-format setting a bit
more in WebAssembly#1986, WebAssembly#2038, and WebAssembly#2044, but mostly I don't have strong
opinions.
This PR is intended only for code review and should not be merged to the
codebase. After this is reviewed and approved, I ask @kripken to create
a separate PR with the same diff and merge it, not to overwrite the
whole codebase with my username, who is not a very frequent contributor
here.
I also plan to add a hook that checks if the PR is properly
clang-formatted to our Travis CI, but I'm gonna test it in another PR
first.
aheejin
added a commit
to aheejin/binaryen
that referenced
this pull request
Apr 24, 2019
This applies clang-format to the whole codebase. Below are manual tweaks
I had to make:
1. After clang-format changed the order of #includes, the code failed to
compile, so this also adds some missing header guards and more #includes
to make code compile.
2. After applying clang-format to this kind of code,
```
cashew::IString GLOBAL("global"),
NAN_("NaN"),
INFINITY_("Infinity"),
NAN__("nan"),
INFINITY__("infinity"),
TOPMOST("topmost"),
INT8ARRAY("Int8Array"),
...
```
This becomes
```
cashew::IString GLOBAL("global"), NAN_("NaN"), INFINITY_("Infinity"),
NAN__("nan"), INFINITY__("infinity"), TOPMOST("topmost"),
INT8ARRAY("Int8Array"), INT16ARRAY("Int16Array"),
...
```
I fixed the formatting by adding the type to every line as follows:
```
cashew::IString GLOBAL("global");
cashew::IString NAN_("NaN");
cashew::IString INFINITY_("Infinity");
cashew::IString NAN__("nan");
cashew::IString INFINITY__("infinity");
cashew::IString TOPMOST("topmost");
cashew::IString INT8ARRAY("Int8Array");
...
```
3. I fixed many parts of the comments for readability. The most
frequently occurring example was this:
```
SomeClass var = 123; // very very very very very very very very very very very very very very very very long comment
```
clang-format blinds transforms this into:
```
Someclass var
= 123; // very very very very very very very very very very very very
// very very very very long comment
```
I fixed these to:
```
// very very very very very very very very very very very very very very
// very very long comment
SomeClass var = 123;
```
4. I ended up using `clang-format off` and `clang-format on` to
explicitly disable clang-format only in one place in
`src/support/safe_integer.cpp`. We couldn't maintain its readability of
the last comment block there.
---
Please let me know if there are any suggestions to this or our current
.clang-format configuration. I tweaked our clang-format setting a bit
more in WebAssembly#1986, WebAssembly#2038, and WebAssembly#2044, but mostly I don't have strong
opinions.
This PR is intended only for code review and should not be merged to the
codebase. After this is reviewed and approved, I ask @kripken to create
a separate PR with the same diff and merge it, not to overwrite the
whole codebase with my username, who is not a very frequent contributor
here.
I also plan to add a hook that checks if the PR is properly
clang-formatted to our Travis CI, but I'm gonna test it in another PR
first.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
While looking at code changes after mass clang-formatting our current
codebase, I think this rather hurts readability than improves it. And
none of preset styles (LLVM, Chrome, Google, Mozilla, and WebKit) allows
these, with only exception that Google allows short ifs on a single
line.