[PATCH] D10725: Improve testing for the C API

[Eric Christopher via llvm-commits]

echristo added a comment.

This is starting to shape up nicely. I've got a few more inline nits, and one generic one which is "Could you please document the functions?" Basically document what the arguments and return values are producing and used for.

Thanks!

-eric


================
Comment at: tools/llvm-c-test/echo.cpp:82
@@ +81,3 @@
+        LLVMGetParamTypes(Src, Params);
+        for (unsigned i = 0; i < ParamCount; i++) {
+          Params[i] = clone_type(Params[i], Ctx);
----------------
Can you try to explain a different way? I'm not sure what you mean.

================
Comment at: tools/llvm-c-test/echo.cpp:150
@@ +149,3 @@
+  const char *Name = LLVMGetValueName(Src);
+
+  // First, the value may be constant.
----------------
Then you should probably add asserts. Also, the coding convention is complete sentences for comments.

Something as simple as "Try to clone everything in the llvm::Value hierarchy." is a start on commenting the function.

================
Comment at: tools/llvm-c-test/echo.cpp:218
@@ +217,3 @@
+    case LLVMAlloca: {
+      // TODO: Add a binding for AllocaInst::getAllocatedType
+      LLVMTypeRef Ty = LLVMGetElementType(LLVMTypeOf(Src));
----------------
Go ahead and remove the "add a new binding" todos as they're not relevant here.

================
Comment at: tools/llvm-c-test/echo.cpp:227-229
@@ +226,5 @@
+      SmallVector<LLVMValueRef, 8> Args;
+      for (int i = 0; i < ArgCount; i++) {
+        Args.push_back(clone_value(LLVMGetOperand(Src, i), Builder, VMap));
+      }
+
----------------
No braces necessary for a single statement.

================
Comment at: tools/llvm-c-test/echo.cpp:270
@@ +269,3 @@
+  while(true) {
+    clone_value(Cur, Builder, VMap);
+    Next = LLVMGetNextInstruction(Cur);
----------------
Should probably have a clone_instruction separated out from clone_value so that anything weird is handled?

================
Comment at: tools/llvm-c-test/echo.cpp:283
@@ +282,3 @@
+    if (Prev != Cur) {
+      fprintf(stderr, "Next.Previous instruction is not Current\n");
+      exit(-1);
----------------
Can you make this an error that's clear about what is going on? Ditto with the rest of them in this function, clone_bbs, etc.

================
Comment at: tools/llvm-c-test/echo.cpp:334-337
@@ +333,6 @@
+  unsigned Count = LLVMCountParams(Src);
+  if (Count != LLVMCountParams(Dst)) {
+    fprintf(stderr, "Parameter count mismatch\n");
+    exit(-1);
+  }
+
----------------
Can you add a test verifying this is working to your test?

================
Comment at: tools/llvm-c-test/echo.cpp:402
@@ +401,3 @@
+  if (Count != 0) {
+    fprintf(stderr, "Parameter count does not match iterration\n");
+    exit(-1);
----------------
"iteration"


http://reviews.llvm.org/D10725