[libc-commits] [PATCH] D92137: [libc] add tests to WrapperGen

[Siva Chandra via Phabricator via libc-commits]

sivachandra added inline comments.


================
Comment at: libc/test/utils/CMakeLists.txt:3
 add_subdirectory(CPP)
+add_subdirectory(WrapperGen)
----------------
Should there be an additional nesting `tools/WrapperGen`?


================
Comment at: libc/test/utils/WrapperGen/CMakeLists.txt:1
+function(add_libc_tool_unittest target_name)
+  if(NOT LLVM_INCLUDE_TESTS)
----------------
The implementation is currently wrappergen specific. So, may be make it generic and move it up one level higher under `libc/test/utils/tools`?


================
Comment at: libc/test/utils/WrapperGen/CMakeLists.txt:10
+    "" # Single value arguments
+    "SRCS;DEPENDS" # Multi-value arguments
+    ${ARGN}
----------------
Add `ARGS` or something equivalent - See below.


================
Comment at: libc/test/utils/WrapperGen/CMakeLists.txt:29
+    COMMAND $<TARGET_FILE:${target_name}> 
+            --path=${LIBC_SOURCE_DIR}
+            --wrappergen=${CMAKE_BINARY_DIR}/bin/libc-wrappergen
----------------
Instead of listing `--path`, get the command line args from the arguments to `add_libc_tool_unittest`.


================
Comment at: libc/test/utils/WrapperGen/CMakeLists.txt:30
+            --path=${LIBC_SOURCE_DIR}
+            --wrappergen=${CMAKE_BINARY_DIR}/bin/libc-wrappergen
+            --targetos=${LIBC_TARGET_OS}
----------------
Give a generic name like, say `--tool`?


================
Comment at: libc/test/utils/WrapperGen/CMakeLists.txt:31
+            --wrappergen=${CMAKE_BINARY_DIR}/bin/libc-wrappergen
+            --targetos=${LIBC_TARGET_OS}
+  )
----------------
Same as for `--path`.


================
Comment at: libc/test/utils/WrapperGen/CMakeLists.txt:45
+
+add_dependencies(wrappergen_test libc-wrappergen)
+
----------------
This should be listed under `DEPENDS` of `add_libc_tool_unittest`?


================
Comment at: libc/test/utils/WrapperGen/CMakeLists.txt:48
+target_compile_options(wrappergen_test PUBLIC -fno-rtti)
+target_link_libraries(wrappergen_test PRIVATE LLVMSupport)
----------------
You can move lines 47 and 48 into `add_libc_tool_unittest` function.

Also, use fully qualified names: https://github.com/llvm/llvm-project/blob/master/libc/cmake/modules/LLVMLibCTargetNameUtils.cmake


================
Comment at: libc/test/utils/WrapperGen/wrappergen_test.cpp:27
+    llvm::cl::value_desc("<path to wrappergen>"), llvm::cl::Required);
+llvm::cl::opt<std::string>
+    TargetOS("targetos", llvm::cl::desc("Name of the target operating system."),
----------------
Instead of this, we should perhaps have an option called `apifile` for which we pass in a test API file. This test file need not do anything fancy. Something like:

```
include "spec/spec.td"
include "spec/std.td"
```


================
Comment at: libc/test/utils/WrapperGen/wrappergen_test.cpp:70
+
+TEST_F(WrapperGenTest, RunWrapperGen) {
+  llvm::Optional<llvm::StringRef> Redirects[] = {
----------------
Also add a test for aliases.


================
Comment at: libc/test/utils/WrapperGen/wrappergen_test.cpp:105
+                          "}\n");
+  // TODO:(michaelrj) Figure out how to make this less brittle.
+}
----------------
Can you list the brittle points and add TODOs separately for them?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D92137/new/

https://reviews.llvm.org/D92137