[PATCH] D11691: [WebAssembly] Add Relooper

JF Bastien jfb at chromium.org
Fri Jul 31 13:35:50 PDT 2015


jfb added a comment.

Could you also add the `.cpp` file to `CMakeLists.txt`? The `Makefile` picks it up automatically...


================
Comment at: lib/Target/WebAssembly/Relooper.h:19
@@ +18,3 @@
+#include <stdio.h>
+#include <stdarg.h>
+
----------------
It's more common to include the C++ version:
```
#include <cassert>
#include <cstdio>
#include <cstdarg>
```

================
Comment at: lib/Target/WebAssembly/Relooper.h:27
@@ +26,3 @@
+struct Block;
+struct Shape;
+
----------------
You should put everything in a sub-namespace of the `llvm` namespace.

================
Comment at: lib/Target/WebAssembly/Relooper.h:41
@@ +40,3 @@
+  };
+  Shape *Ancestor; // If not NULL, this shape is the relevant one for purposes
+                   // of getting to the target block. We break or continue on it
----------------
`nullptr` is the new hotness, here and elsewhere.

================
Comment at: lib/Target/WebAssembly/Relooper.h:60
@@ +59,3 @@
+// of operator<(T, T)). This is useful to ensure deterministic
+// behavior.
+template <typename T> struct InsertOrderedSet {
----------------
Could you use `llvm/ADT/SetVector.h` instead?

================
Comment at: lib/Target/WebAssembly/Relooper.h:104
@@ +103,3 @@
+// of operator<(Key, Key)). This is useful to ensure deterministic
+// behavior.
+template <typename Key, typename T> struct InsertOrderedMap {
----------------
Could you use `llvm/ADT/SetVector.h` instead?


Repository:
  rL LLVM

http://reviews.llvm.org/D11691







More information about the llvm-commits mailing list