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

[Paula Toth via Phabricator via libc-commits]

PaulkaToast added inline comments.


================
Comment at: libc/config/linux/entrypoints.txt:1
-
-add_entrypoint_library(
-  llvmlibc
-  DEPENDS
+set(LIBC_ENTRYPOINTS
     # assert.h entrypoints
----------------
sivachandra wrote:
> PaulkaToast wrote:
> > 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.
> My comment is more for our convenience while we are building the libc for different target machine architectures. Having just one linux list means that while the implementation for other architectures is incomplete, our build infrastructure will keep failing at different places.
Ah, this is a good point. If you are porting to another architecture and building it out its much more convenient to do it in pieces, a few entrpoints at a time, which multiple lists allow. Done.


================
Comment at: libc/utils/HdrGen/PublicAPICommand.h:42
 
+class APIReader {
+public:
----------------
sivachandra wrote:
> PaulkaToast wrote:
> > 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?
> Yes. Refactor it separately. It need not really be a refactor. I am OK even if you just make API generator public. As it stands in this patch, the specialization and the base class have no meaning as `makeAPIReader` is really not a factory method.
> 
> Splitting the CMake parts and the hdrgen parts into two patches makes it easier for me to review as well.
Okay, this is now pulled into a desperate patch: D80832.
When that patch gets merged in I'll pull it in here and update the diff.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79192