[libc-commits] [PATCH] D79192: [libc] Add integration tests.

[Siva Chandra via Phabricator via libc-commits]

sivachandra added inline comments.


================
Comment at: libc/CMakeLists.txt:85
 endif()
+add_subdirectory(lib)
----------------
Why is this change required?


================
Comment at: libc/cmake/modules/LLVMLibCLibraryRules.cmake:93
   get_object_files_for_entrypoint_library(obj_list ${fq_deps_list})
+  set(entrypoint_list  "")
   foreach(dep IN LISTS fq_deps_list)
----------------
Call this entry point *name* list throughout to avoid confusion with target names.


================
Comment at: libc/cmake/modules/LLVMLibCObjectRules.cmake:1
 set(OBJECT_LIBRARY_TARGET_TYPE "OBJECT_LIBRARY")
 
----------------
Seems to me like the changes in this file should be separated out into a different patch.


================
Comment at: libc/config/linux/entrypoints.txt:1
-
-add_entrypoint_library(
-  llvmlibc
-  DEPENDS
+set(LIBC_ENTRYPOINTS
     # assert.h entrypoints
----------------
This LGTM. But, do you think it makes sense to put this in a target machine specific directory (`config/linux/x86_64/entrypoints.txt`)? For example, almost everything under `LIBC_ENTRYPOINTS` currently is x86_64 specific. So, putting these in a machine specific list helps us build the libc for different machine types independent of each of other.


================
Comment at: libc/config/linux/headers.txt:1
+set(PUBLIC_HEADERS
+    libc.include.assert_h
----------------
The same would be true for headers also I suppose but we can do that separately as it does not interfere with the current patch?


================
Comment at: libc/lib/CMakeLists.txt:22
+
+add_libc_public_integration_test(
+  public_integration_test
----------------
AFAICT, this is the only place this rule is being used. Do you see it being used anywhere else? If not, just inline everything here. May be even put it under the `test` directory and not here?


================
Comment at: libc/utils/HdrGen/PublicAPICommand.h:42
 
+class APIReader {
+public:
----------------
I see only one specialization of this class. In which case, why is this need?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79192