Address review feedback for #1014 (#1016)

* address review feedback for #1014

Author: kripken

check.py

@@ -148,6 +148,9 @@ def do_asm2wasm_test():
         finally:
           if old_pass_debug is not None:
             os.environ['BINARYEN_PASS_DEBUG'] = old_pass_debug
+          else:
+            if 'BINARYEN_PASS_DEBUG' in os.environ:
+              del os.environ['BINARYEN_PASS_DEBUG']
 
         # verify in wasm
         if options.interpreter:

src/ast/ExpressionManipulator.cpp

@@ -19,7 +19,9 @@
 
 namespace wasm {
 
-Expression* ExpressionManipulator::flexibleCopy(Expression* original, Module& wasm, CustomCopier custom) {
+namespace ExpressionManipulator {
+
+Expression* flexibleCopy(Expression* original, Module& wasm, CustomCopier custom) {
   struct Copier : public Visitor<Copier, Expression*> {
     Module& wasm;
     CustomCopier custom;
@@ -135,7 +137,7 @@ Expression* ExpressionManipulator::flexibleCopy(Expression* original, Module& wa
 
 
 // Splice an item into the middle of a block's list
-void ExpressionManipulator::spliceIntoBlock(Block* block, Index index, Expression* add) {
+void spliceIntoBlock(Block* block, Index index, Expression* add) {
   auto& list = block->list;
   if (index == list.size()) {
     list.push_back(add); // simple append
@@ -150,4 +152,6 @@ void ExpressionManipulator::spliceIntoBlock(Block* block, Index index, Expressio
   block->finalize(block->type);
 }
 
+} // namespace ExpressionManipulator
+
 } // namespace wasm

src/ast/branch-utils.h

@@ -0,0 +1,44 @@
+/*
+ * Copyright 2017 WebAssembly Community Group participants
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#ifndef wasm_ast_branch_h
+#define wasm_ast_branch_h
+
+#include "wasm.h"
+
+namespace wasm {
+
+namespace BranchUtils {
+
+// branches not actually taken (e.g. (br $out (unreachable)))
+// are trivially ignored in our type system
+
+inline bool isBranchTaken(Break* br) {
+  return !(br->value     && br->value->type     == unreachable) &&
+         !(br->condition && br->condition->type == unreachable);
+}
+
+inline bool isBranchTaken(Switch* sw) {
+  return !(sw->value && sw->value->type     == unreachable) &&
+                        sw->condition->type != unreachable;
+}
+
+} // namespace BranchUtils
+
+} // namespace wasm
+
+#endif // wasm_ast_branch_h
+

src/ast/manipulation.h

@@ -21,10 +21,10 @@
 
 namespace wasm {
 
-struct ExpressionManipulator {
+namespace ExpressionManipulator {
   // Re-use a node's memory. This helps avoid allocation when optimizing.
   template<typename InputType, typename OutputType>
-  static OutputType* convert(InputType *input) {
+  inline OutputType* convert(InputType *input) {
     static_assert(sizeof(OutputType) <= sizeof(InputType),
                   "Can only convert to a smaller size Expression node");
     input->~InputType(); // arena-allocaed, so no destructor, but avoid UB.
@@ -35,13 +35,13 @@ struct ExpressionManipulator {
 
   // Convenience method for nop, which is a common conversion
   template<typename InputType>
-  static Nop* nop(InputType* target) {
+  inline Nop* nop(InputType* target) {
     return convert<InputType, Nop>(target);
   }
 
   // Convert a node that allocates
   template<typename InputType, typename OutputType>
-  static OutputType* convert(InputType *input, MixedArena& allocator) {
+  inline OutputType* convert(InputType *input, MixedArena& allocator) {
     assert(sizeof(OutputType) <= sizeof(InputType));
     input->~InputType(); // arena-allocaed, so no destructor, but avoid UB.
     OutputType* output = (OutputType*)(input);
@@ -50,18 +50,18 @@ struct ExpressionManipulator {
   }
 
   using CustomCopier = std::function<Expression*(Expression*)>;
-  static Expression* flexibleCopy(Expression* original, Module& wasm, CustomCopier custom);
+  Expression* flexibleCopy(Expression* original, Module& wasm, CustomCopier custom);
 
-  static Expression* copy(Expression* original, Module& wasm) {
+  inline Expression* copy(Expression* original, Module& wasm) {
     auto copy = [](Expression* curr) {
         return nullptr;
     };
     return flexibleCopy(original, wasm, copy);
   }
 
   // Splice an item into the middle of a block's list
-  static void spliceIntoBlock(Block* block, Index index, Expression* add);
-};
+  void spliceIntoBlock(Block* block, Index index, Expression* add);
+}
 
 } // wasm
 

src/ast/type-updating.h

@@ -132,13 +132,11 @@ struct TypeUpdater : public ExpressionStackWalker<TypeUpdater, UnifiedExpression
   // adds (or removes) breaks depending on break/switch contents
   void discoverBreaks(Expression* curr, int change) {
     if (auto* br = curr->dynCast<Break>()) {
-      if (!(br->value     && br->value->type     == unreachable) &&
-          !(br->condition && br->condition->type == unreachable)) {
+      if (BranchUtils::isBranchTaken(br)) {
         noteBreakChange(br->name, change, br->value);
       }
     } else if (auto* sw = curr->dynCast<Switch>()) {
-      if (!(sw->value     && sw->value->type     == unreachable) &&
-                             sw->condition->type != unreachable) {
+      if (BranchUtils::isBranchTaken(sw)) {
         applySwitchChanges(sw, change);
       }
     }

src/ast_utils.h

@@ -21,6 +21,7 @@
 #include "wasm-traversal.h"
 #include "wasm-builder.h"
 #include "pass.h"
+#include "ast/branch-utils.h"
 
 namespace wasm {
 
@@ -371,24 +372,19 @@ struct ReFinalize : public WalkerPass<PostWalker<ReFinalize>> {
   void visitLoop(Loop *curr) { curr->finalize(); }
   void visitBreak(Break *curr) {
     curr->finalize();
-    if (curr->value && curr->value->type == unreachable) {
-      return; // not an actual break
+    if (BranchUtils::isBranchTaken(curr)) {
+      breakValues[curr->name] = getValueType(curr->value);
     }
-    if (curr->condition && curr->condition->type == unreachable) {
-      return; // not an actual break
-    }
-    breakValues[curr->name] = getValueType(curr->value);
   }
   void visitSwitch(Switch *curr) {
     curr->finalize();
-    if (curr->condition->type == unreachable || (curr->value && curr->value->type == unreachable)) {
-      return; // not an actual break
-    }
-    auto valueType = getValueType(curr->value);
-    for (auto target : curr->targets) {
-      breakValues[target] = valueType;
+    if (BranchUtils::isBranchTaken(curr)) {
+      auto valueType = getValueType(curr->value);
+      for (auto target : curr->targets) {
+        breakValues[target] = valueType;
+      }
+      breakValues[curr->default_] = valueType;
     }
-    breakValues[curr->default_] = valueType;
   }
   void visitCall(Call *curr) { curr->finalize(); }
   void visitCallImport(CallImport *curr) { curr->finalize(); }

src/passes/MergeBlocks.cpp

@@ -224,7 +224,7 @@ struct MergeBlocks : public WalkerPass<PostWalker<MergeBlocks>> {
     if (auto* block = child->dynCast<Block>()) {
       if (!block->name.is() && block->list.size() >= 2) {
         child = block->list.back();
-        // we modified child )which is *&), which modifies curr, which might change its type
+        // we modified child (which is a reference to a pointer), which modifies curr, which might change its type
         // (e.g. (drop (block i32 .. (unreachable)))
         // the child was a block of i32, and is being replaced with an unreachable, so the
         // parent will likely need to be unreachable too
@@ -277,7 +277,7 @@ struct MergeBlocks : public WalkerPass<PostWalker<MergeBlocks>> {
     if (EffectAnalyzer(getPassOptions(), curr->ifTrue).hasSideEffects()) return;
     outer = optimize(curr, curr->ifFalse, outer);
     if (EffectAnalyzer(getPassOptions(), curr->ifFalse).hasSideEffects()) return;
-    /* . */ optimize(curr, curr->condition, outer);
+            optimize(curr, curr->condition, outer);
   }
 
   void visitDrop(Drop* curr) {

src/passes/Print.cpp

@@ -25,7 +25,7 @@
 
 namespace wasm {
 
-static int forceFull() {
+static int isFullForced() {
   if (getenv("BINARYEN_PRINT_FULL")) {
     return std::stoi(getenv("BINARYEN_PRINT_FULL"));
   }
@@ -48,7 +48,7 @@ struct PrintSExpression : public Visitor<PrintSExpression> {
 
   PrintSExpression(std::ostream& o) : o(o) {
     setMinify(false);
-    if (!full) full = forceFull();
+    if (!full) full = isFullForced();
   }
 
   void visit(Expression* curr) {
@@ -807,7 +807,7 @@ std::ostream& WasmPrinter::printExpression(Expression* expression, std::ostream&
   }
   PrintSExpression print(o);
   print.setMinify(minify);
-  if (full || forceFull()) {
+  if (full || isFullForced()) {
     print.setFull(true);
     o << "[" << printWasmType(expression->type) << "] ";
   }

src/wasm-validator.h

@@ -43,6 +43,7 @@
 #include "wasm.h"
 #include "wasm-printing.h"
 #include "ast_utils.h"
+#include "ast/branch-utils.h"
 
 namespace wasm {
 
@@ -230,8 +231,7 @@ struct WasmValidator : public PostWalker<WasmValidator> {
   }
   void visitBreak(Break *curr) {
     // note breaks (that are actually taken)
-    if ((!curr->value     || curr->value->type     != unreachable) &&
-        (!curr->condition || curr->condition->type != unreachable)) {
+    if (BranchUtils::isBranchTaken(curr)) {
       noteBreak(curr->name, curr->value, curr);
     }
     if (curr->condition) {
@@ -240,7 +240,7 @@ struct WasmValidator : public PostWalker<WasmValidator> {
   }
   void visitSwitch(Switch *curr) {
     // note breaks (that are actually taken)
-    if (curr->condition->type != unreachable && (!curr->value || curr->value->type != unreachable)) {
+    if (BranchUtils::isBranchTaken(curr)) {
       for (auto& target : curr->targets) {
         noteBreak(target, curr->value, curr);
       }