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

[Michael Jones via Phabricator via libc-commits]

michaelrj added a comment.

submit comments



================
Comment at: libc/test/utils/WrapperGen/CMakeLists.txt:45
+
+add_dependencies(wrappergen_test libc-wrappergen)
+
----------------
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.


================
Comment at: libc/test/utils/WrapperGen/CMakeLists.txt:48
+target_compile_options(wrappergen_test PUBLIC -fno-rtti)
+target_link_libraries(wrappergen_test PRIVATE LLVMSupport)
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92137