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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
