[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/WrapperGen/CMakeLists.txt:45
+
+add_dependencies(wrappergen_test libc-wrappergen)
+
----------------
michaelrj wrote:
> sivachandra wrote:
> > This should be listed under `DEPENDS` of `add_libc_tool_unittest`?
> no, if `libc-wrappergen` is part of the DEPENDS block it tries to link it in as part of the tests, which causes a bunch of errors, thus it's added separately.
So, IIUC, `add_libc_tool_unittest` expects an optional `DEPENDS` argument but does nothing with it. If `DEPENDS` is confusing, use `TOOL_TARGET` may be and then you can call `add_dependencies` to add this tool as a dependency.


================
Comment at: libc/test/utils/WrapperGen/CMakeLists.txt:48
+target_compile_options(wrappergen_test PUBLIC -fno-rtti)
+target_link_libraries(wrappergen_test PRIVATE LLVMSupport)
----------------
michaelrj wrote:
> sivachandra wrote:
> > 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
> I'm not quite sure what you mean by "use fully qualified names" since the file you linked appears to only support paths inside libc and LLVMSupport is from https://github.com/llvm/llvm-project/blob/master/llvm/lib/Support/CMakeLists.txt#L79 unless you meant a different name?
What I mean is, the `wrappergen_test` target should have a fully qualified name like `libc.test.utils.tools.WrapperGen.wrappergen_test`.


================
Comment at: libc/test/utils/tools/CMakeLists.txt:2
+function(add_libc_tool_unittest target_name)
+  if(NOT LLVM_INCLUDE_TESTS)
+    return()
----------------
You shouldn't need this because: https://github.com/llvm/llvm-project/blob/master/libc/CMakeLists.txt#L97


================
Comment at: libc/test/utils/tools/CMakeLists.txt:38
+add_subdirectory(WrapperGen)
\ No newline at end of file

----------------
Fix.


================
Comment at: libc/test/utils/tools/WrapperGen/CMakeLists.txt:6
+    wrappergen_test.cpp   
+  DEPENDS
+  ARGS
----------------
Why list an empty `DEPENDS`?


================
Comment at: libc/test/utils/tools/WrapperGen/wrappergen_test.cpp:27
+             llvm::cl::value_desc("<path to tool>"), llvm::cl::Required);
+llvm::cl::opt<std::string>
+    TargetOS("targetos", llvm::cl::desc("Name of the target operating system."),
----------------
We should make this test independent of the target OS/machine. That is why I suggested using a test API file. Did you run into any problems with that approach?


================
Comment at: libc/test/utils/tools/WrapperGen/wrappergen_test.cpp:48
+    APIArg.append(TargetOS);
+    APIArg.append("/api.td");
+    ProgPath = llvm::StringRef(ToolPath);
----------------
If you use the test API file approach, you will not need to construct the path to the API file. Just use the path as listed on the command line.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92137