[PATCH] D10725: Improve testing for the C API

[Eric Christopher via llvm-commits]

echristo added a comment.

OK, I've started in on the inline comments. I started adding one about naming conventions on the functions, but I notice now that this just goes with the rest of the files - which is fine I guess. In the C++ code, as much as possible, do use the llvm naming/style conventions.

Thanks!

-eric


================
Comment at: tools/llvm-c-test/calc.c:17
@@ -16,3 +16,2 @@
 #include "llvm-c-test.h"
-#include "llvm-c/Core.h"
 #include <stdio.h>
----------------
Commit this separately if it isn't necessary.

================
Comment at: tools/llvm-c-test/echo.cpp:10-11
@@ +9,4 @@
+//
+// This file implements the --echo commands in llvm-c-test.
+//
+//===----------------------------------------------------------------------===//
----------------
Please elaborate here on what this testing tool is doing and how it's testing things.

================
Comment at: tools/llvm-c-test/echo.cpp:17-18
@@ +16,4 @@
+
+#include <stdio.h>
+#include <stdlib.h>
+
----------------
Shouldn't be necessary anymore?

================
Comment at: tools/llvm-c-test/echo.cpp:20
@@ +19,3 @@
+
+#include "llvm/ADT/DenseMap.h"
+
----------------
Move this with the rest of the llvm includes.

================
Comment at: tools/llvm-c-test/echo.cpp:25
@@ +24,3 @@
+// Provide DenseMapInfo for C API opaque types.
+template<typename T>
+struct CAPIDenseMap {};
----------------
You changed it to use a DenseMap, but I'm curious why?

================
Comment at: tools/llvm-c-test/echo.cpp:76-80
@@ +75,7 @@
+      if (ParamCount > 0) {
+        Params = (LLVMTypeRef*) malloc(ParamCount * sizeof(LLVMTypeRef));
+        LLVMGetParamTypes(Src, Params);
+        for (unsigned i = 0; i < ParamCount; i++) {
+          Params[i] = clone_type(Params[i], Ctx);
+        }
+      }
----------------
This is C++ now yes?

================
Comment at: tools/llvm-c-test/echo.cpp:148
@@ +147,3 @@
+
+  // Maybe it is a contant
+  if (LLVMIsAConstant(Src)) {
----------------
Can you make the comments here more elaborate of what you're looking for here?

================
Comment at: tools/llvm-c-test/echo.cpp:300-302
@@ +299,5 @@
+  unsigned Count = LLVMCountBasicBlocks(Src);
+  if (Count == 0) {
+    return;
+  }
+
----------------
llvm coding style is no braces around single line blocks. (And in other places).


http://reviews.llvm.org/D10725