[libc-commits] [PATCH] D91394: [libc] Switch functions to using global headers

[Siva Chandra via Phabricator via libc-commits]

sivachandra added a comment.

I think Michael is trying to satisfy a Fuchsia use case which is effectively like taking sources from LLVM libc and dropping them into Fuchsia's libc. In which case, the internal paths currently used by LLVM libc will not work. After going through all the combinations in this change, I think we should probably not do this for all headers. Take `threads.h` for example. It has type definitions which cannot be mixed with `threads.h` from elsewhere. This is effectively what Fangrui is pointing out - mixing is fraught with landmines.

So, Michael, we should probably limit this to just `math.h` and `string.h` as the functions from these headers are not ABI or platform sensitive. That is, you should only replace `#include "include/math.h"` with `#include <math.h>`, and `#include "include/string.h` with `#include <string.h>` from within implementations of `math` and `string` functions only. Also, I think the current includes of `include/string.h` can be replaced with includes of `stddef.h`? If so, then may be do that instead of including the public header.

By limiting to just `string.h` and `math.h`, we are not only saying that it is OK to take sources of LLVM libc's `string.h` and math.h` functions and drop them into another libc's source tree, we are also saying that doing the same with other functions is not OK. That doesn't mean this situation is completely fine. We need to have a more principled way of doing this, at the least add a document somewhere explaining this inconsistency. LLVM libc'c build itself is protected by the clang tidy check llvmlibc-restrict-system-libc-headers. May be, we should additionally have some check which catches inclusion of public headers in implementations of ABI sensitive functions. All of these can be taken up outside this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91394