This switches all of the files in src/string, src/math, and
test/src/math from using relative paths (e.g. #include “include/string.h”)
to global paths (e.g. #include <string.h>) to make bringing up those
functions on other platforms, such as fuchsia, easier.
Details
- Reviewers
sivachandra - Commits
- rG3e18fb339039: [libc] Switch functions to using global headers
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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.
I will consider this experimental for now. We should follow up with a plan for a tool based build time guidance which will tell us if including the header <...> is OK.