Don't binpack arguments and parameters #2038
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
Looking at the current codebase, we don't mostly binpack arguments and
parameters when they don't fit in a single line.
BinPackArguments:
```
true:
void f() {
f(aaaaaaaaaaaaaaaaaaaa, aaaaaaaaaaaaaaaaaaaa,
aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa);
}
false:
void f() {
f(aaaaaaaaaaaaaaaaaaaa,
aaaaaaaaaaaaaaaaaaaa,
aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa);
}
```
BinPackParameters:
```
true:
void f(int aaaaaaaaaaaaaaaaaaaa, int aaaaaaaaaaaaaaaaaaaa,
int aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa) {}
false:
void f(int aaaaaaaaaaaaaaaaaaaa,
int aaaaaaaaaaaaaaaaaaaa,
int aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa) {}
```
Member
Author
|
Looking at the ways to make formatting as much similar as to the current codebase, and this is one thing I saw. The other thing I'm not sure about is Align: DontAlign: AlwaysBreak: Currently we mostly do is possible. What do you think you prefer? |
kripken
approved these changes
Apr 22, 2019
Member
kripken
left a comment
kripken
left a comment
There was a problem hiding this comment.
Yeah, I think that's more common here. Also looks better in my opinion.
Member
Author
|
@kripken Thanks. Did you see my comment about |
Member
|
Oh, I just saw the |
Member
Author
|
Hmm, after looking at some more code, I found that whether we binpack arguments in function calls or not is like 50:50, and we mostly rather binpack parameters. Should we remove |
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.
Looking at the current codebase, we mostly don't binpack arguments and
parameters when they don't fit in a single line.
BinPackArguments:
BinPackParameters: