Format all the things by kripken · Pull Request #1435 · WebAssembly/binaryen

kripken · 2018-02-21T00:08:52Z

Followup for #1407. This runs clang-format and clang-tidy on everything.

Downsides:

Long-term loss of git-blame effectiveness.
Short-term annoyance in getting this merged and other PRs in progress conflicting with it.
This is my first time using these two clang tools. I am somewhat concerned after seeing that I needed to fix these two things up: d5a6ccb#diff-20f762851bbf02620da8d3164ff6dd7bL36 b42b303 - luckily those were also compilation errors, but others might not be. And a full review of all the changes here to look for bugs is impractical due to the size.
Also it does fun things to EM_ASMs, but can't blame it there ;) 8b94451

Thoughts on if this is a good idea?

done with clang-tidy -checks="-*,readability-braces-around-statements" `find -name "*.cpp"` -fix-errors -- -xc++ -std=c++11 -I. -w etc.

find ./ -name '*.cpp' -or -name '*.h' | xargs ~/Dev/fastcomp/build/bin/clang-format -i

sbc100 · 2018-02-21T01:52:10Z

Do you know it was was clang-tidy or clang-format that introduced the bugs.

My understanding of clang-format at least is that this should be impossible. Not sure thats true of clang-tidy which I guess can do more complex transforms. If it was clang-format that did it, its probably worth filing a bug.

sbc100 · 2018-02-21T01:54:30Z

-    case 'd': return f64;
-    case 'v': return none;
-    default: abort();
+    case 'i':


I wonder if we can find a setting to allow this kind of thing?

Setting AllowShortCaseLabelsOnASingleLine to true allows this, but settings also means converting all two-line case statements into a single line if they fit in 120 cols, the current ColumnLimit. There are several options like this; refer to the bottom of #1407 (comment) for details.

So we have to either convert all

case 'i': blahblah;

into

case 'i': blahblah;

or we have to convert all

case 'i': blahblah;

into

case 'i': blahblah;

So the point is, even though the option name is AllowShortCaseLabelsOnASingleLine, there is no option that allows something. clang-format, in nature, forces a single style for given code and configurations. The option should read as ForceShortCaseLabelsOnASingleLine.

Merging all two-line case statements (or if statements, ...) into a single line may not sound that bad, but we also have ColumnLimit of 120, which is longer than usual. This will merge a lot of two-line case statements into one. So, I was not able to decide, so I ended up not adding those Allow*** options. We can add them if people prefer that.

sbc100

Is it worth doing the clang-format and clang-tidy separately? (I'm not sure exactly what the distinctions are, but I get the impression clang-format is safer, since its limited to basically whitespace only changes).

sbc100 · 2018-02-21T01:54:44Z

-                WASM_ROTR64("__wasm_rotr_i64"),
-                WASM_GROW_MEMORY("__wasm_grow_memory"),
-                WASM_CURRENT_MEMORY("__wasm_current_memory");
+cashew::IString GLOBAL("global"), NAN_("NaN"), INFINITY_("Infinity"), NAN__("nan"),


This one seems pretty bad

When comparing changes in #1407, I also noticed this, and I couldn't help it. The only option that allows multi-line definition in this case was ColumnLimit = 0, which effectively enforces no limit on the column length. Funnily, this allows multi-line definitions. Don't know why.

sbc100 · 2018-02-21T01:56:04Z

-                WASM_ROTR64,
-                WASM_GROW_MEMORY,
-                WASM_CURRENT_MEMORY;
+extern cashew::IString GLOBAL, NAN_, INFINITY_, NAN__, INFINITY__, TOPMOST, INT8ARRAY, INT16ARRAY,


This too. I wonder if this could should be done via a "#include .def" thing?

sbc100 · 2018-02-21T01:59:06Z

-    return
+    return builder.makeBinary(OrInt64, builder.makeUnary(ExtendUInt32, low),
      builder.makeBinary(
-        OrInt64,


Is it OK to looks this structure?

I'm not happy about this change, yeah.

sbc100 · 2018-02-21T02:00:48Z

+  int optimizeLevel = 0;         // 0, 1, 2 correspond to -O0, -O1, -O2, etc.
+  int shrinkLevel = 0;           // 0, 1, 2 correspond to -O0, -Os, -Oz
+  bool ignoreImplicitTraps =
+    false;                // optimize assuming things like div by 0, bad load/store, will not trap


This looks a but odd.

aheejin · 2018-02-21T02:10:44Z

@sbc100 To my understanding, the reason @kripken wanted clang-tidy was he wanted to ensure there is always enclosing braces for single-line ifs and loops. So, he wanted to convert

if (condition)
  statement;

to

if (condition) {
  statement;
}

aheejin · 2018-02-21T02:17:42Z

I haven't checked all the code changes yet, but I also wonder if the bugs are caused by clang-format or clang-tidy. IIUC clang-format cannot change characters in code except whitespaces.

I also agree that this might be hard to test. Applying the style incrementally, by recommending people to apply clang-format on the part of the code they modify using git clang-format, might be a better option.

kripken · 2018-02-21T03:27:36Z

Looks like it was clang-tidy that caused both of those bugs.

kripken · 2018-04-03T00:08:10Z

Probably doens't make sense to do this. I'll leave the branch though if we want to reconsider this in the future.

kripken added 7 commits February 20, 2018 13:29

apply clang-format

aad66b0

clang-tidy up missing braces

1fe41f6

done with clang-tidy -checks="-*,readability-braces-around-statements" `find -name "*.cpp"` -fix-errors -- -xc++ -std=c++11 -I. -w etc.

run clang-format after clang-tidy, something like

af1a19f

find ./ -name '*.cpp' -or -name '*.h' | xargs ~/Dev/fastcomp/build/bin/clang-format -i

fix a clang-format/tidy-introduced bug

d5a6ccb

fix another clang-format/tidy-introduced bug

b42b303

header fixes

60556e1

fix up EM_ASMs

8b94451

kripken mentioned this pull request Feb 21, 2018

Add .clang-format #1407

Merged

sbc100 reviewed Feb 21, 2018

View reviewed changes

kripken closed this Apr 3, 2018

aheejin mentioned this pull request Apr 5, 2019

clang-format revisited + line length of 80 cols #1981

Closed

sbc100 deleted the formatting branch April 10, 2019 18:29

aheejin mentioned this pull request Apr 25, 2019

Format all the things (Only for review) #2048

Closed

Uh oh!

Conversation

kripken commented Feb 21, 2018

Uh oh!

sbc100 commented Feb 21, 2018

Uh oh!

sbc100 Feb 21, 2018

Choose a reason for hiding this comment

Uh oh!

aheejin Feb 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sbc100 left a comment

Choose a reason for hiding this comment

Uh oh!

sbc100 Feb 21, 2018

Choose a reason for hiding this comment

Uh oh!

aheejin Feb 21, 2018

Choose a reason for hiding this comment

Uh oh!

sbc100 Feb 21, 2018

Choose a reason for hiding this comment

Uh oh!

sbc100 Feb 21, 2018

Choose a reason for hiding this comment

Uh oh!

kripken Feb 21, 2018

Choose a reason for hiding this comment

Uh oh!

sbc100 Feb 21, 2018

Choose a reason for hiding this comment

Uh oh!

aheejin commented Feb 21, 2018

Uh oh!

aheejin commented Feb 21, 2018

Uh oh!

kripken commented Feb 21, 2018

Uh oh!

kripken commented Apr 3, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

aheejin Feb 21, 2018 •

edited

Loading