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

[Paula Toth via Phabricator via libc-commits]

PaulkaToast added inline comments.


================
Comment at: libc/CMakeLists.txt:85
 endif()
+add_subdirectory(lib)
----------------
sivachandra wrote:
> Why is this change required?
Due to the changes below its no longer required (:


================
Comment at: libc/cmake/modules/LLVMLibCObjectRules.cmake:1
 set(OBJECT_LIBRARY_TARGET_TYPE "OBJECT_LIBRARY")
 
----------------
sivachandra wrote:
> Seems to me like the changes in this file should be separated out into a different patch.
Yes, I could pull it out into another patch but this is the only current use case for this change so maybe its better to leave it in this patch? wdyt?


================
Comment at: libc/config/linux/entrypoints.txt:1
-
-add_entrypoint_library(
-  llvmlibc
-  DEPENDS
+set(LIBC_ENTRYPOINTS
     # assert.h entrypoints
----------------
sivachandra wrote:
> 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.
I'm not necessarily opposed to this, although I'm not sure I know of a case where the libc differs for different architectures on linux. If the need arises it should be a very simple change to make if/when we encounter it.


================
Comment at: libc/config/linux/headers.txt:1
+set(PUBLIC_HEADERS
+    libc.include.assert_h
----------------
sivachandra wrote:
> 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?
yes, we can evaluate this in a different patch.


================
Comment at: libc/lib/CMakeLists.txt:22
+
+add_libc_public_integration_test(
+  public_integration_test
----------------
sivachandra wrote:
> 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?
Ah, yes this was useful for when I was testing individual libraries but now there is no need for the function so I'll inline it instead. This also removes the reordering issue from above. Thanks!


================
Comment at: libc/utils/HdrGen/PublicAPICommand.h:42
 
+class APIReader {
+public:
----------------
sivachandra wrote:
> I see only one specialization of this class. In which case, why is this need?
Right now the APIGenerator is defined in the .cpp file, so this is used to expose the parts we want. The alternative is to expose the APIGenerator class in the header, but that'd be a larger refactor. It can be done though, which would you prefer?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79192