[PATCH] D48106: implemented proto to llvm

[Matt Morehouse via Phabricator via cfe-commits]

morehouse added a comment.

In https://reviews.llvm.org/D48106#1131625, @emmettneyman wrote:

> I wanted to implement the proto_to_llvm converter before the fuzz target.


The fuzz target should make testing your converter way easier.  I'd recommend adding it to this patch so that you're less likely to need a bug-fixing patch later.



================
Comment at: tools/clang-fuzzer/proto-to-llvm/loop_proto_to_llvm.cpp:32
+// Counter variable to generate new LLVM IR variable names and wrapper function
+int ctr = 0;
+std::string get_var() {
----------------
You can hide this as a static int inside `get_var`.


================
Comment at: tools/clang-fuzzer/proto-to-llvm/loop_proto_to_llvm.cpp:46
+                   + "store i32 " + val + ", i32* " + alloca_var + "\n"
+                   + load_var + " = load i32, i32* " + alloca_var + "\n";
+  return std::make_pair(insns, load_var);
----------------
What's the point of storing then loading the value?  Can you just return `val`?


================
Comment at: tools/clang-fuzzer/proto-to-llvm/loop_proto_to_llvm.cpp:49
+}
+std::pair <std::string, std::string> VarRefToString(const VarRef &x) {
+  std::string arr;
----------------
Returning a pair can be confusing (which element is which?).  I'd suggest passing `os` to these functions, writing the instructions to `os`, and then returning just the result variable.


================
Comment at: tools/clang-fuzzer/proto-to-llvm/loop_proto_to_llvm.cpp:120
+    op = "add";
+    break;
+  }
----------------
If all these cases are the same, we can simplify the code to
```
case BinaryOp::EQ:
case BinaryOp::NE:
...
op = "add";
break;
```


================
Comment at: tools/clang-fuzzer/proto-to-llvm/loop_proto_to_llvm.cpp:129
+}
+std::string AssignmentStatementToString(const AssignmentStatement &x) {
+  std::pair <std::string, std::string> ref = VarRefToString(x.varref());
----------------
If `AssignmentStatementToString` has no extra return value, I think its more concise to just keep the `operator<<` overload.  (Same for below functions)


Repository:
  rC Clang

https://reviews.llvm.org/D48106