Details
- Reviewers
- None
- Group Reviewers
Restricted Project - Commits
- rG6553608acac4: [libc++] Granulaize math.h and move the functions to std::__math
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
mostly LGTM, but some minor issue.
libcxx/include/__math/remainder.h | ||
---|---|---|
11 | Nit: since the file contains modulo to, how about naming the file math_division.h instead or move the modulo to its own file. | |
libcxx/test/std/depr/depr.c.headers/math_h.pass.cpp | ||
10 ↗ | (On Diff #540445) | Why is this needed, can you mention this in the commit message? |
libcxx/include/__math/remainder.h | ||
---|---|---|
11 | I think moving the modules to their own file sounds good. | |
libcxx/test/std/depr/depr.c.headers/math_h.pass.cpp | ||
10 ↗ | (On Diff #540445) | I'll check again whether it's actually needed. This might just be a leftover from trying to fix something. (I suspect it's not actually needed) |
libcxx/test/std/depr/depr.c.headers/math_h.pass.cpp | ||
---|---|---|
10 ↗ | (On Diff #540445) | It would be great when it's not needed. This feels a bit out of place. |
LGTM, but there is a remark.
libcxx/include/module.modulemap.in | ||
---|---|---|
652 | Please make sure this is rebased on the latest module map format. |
@philnik Changes require descriptions which include the motivation for making the change.
This is not the first time you've been asked to include change descriptions including motivation.
libcxx/include/__math/abs.h | ||
---|---|---|
23 | @philnik Sorry for the late comment but we just noticed that our build is broken with this commit. We do run into name collision with __math being a header guard macro defined into one of our internal headers. Can it be change to have a different name, say __math_impl or something like that? |
libcxx/include/__math/abs.h | ||
---|---|---|
23 |
To clarify it isn't an internal header but the system math.h header. The z/OS system headers tend to use macros like __math or __stdio for the file guard macros. |
@philnik Sorry for the late comment but we just noticed that our build is broken with this commit.
We do run into name collision with __math being a header guard macro defined into one of our internal headers. Can it be change to have a different name, say __math_impl or something like that?