This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Granulaize math.h and move the functions to std::__math
ClosedPublic

Authored by philnik on Jul 13 2023, 9:45 PM.

Details

Reviewers
None
Group Reviewers
Restricted Project
Commits
rG6553608acac4: [libc++] Granulaize math.h and move the functions to std::__math

Diff Detail

Event Timeline

philnik created this revision.Jul 13 2023, 9:45 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 13 2023, 9:45 PM
philnik requested review of this revision.Jul 13 2023, 9:45 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 13 2023, 9:45 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
philnik updated this revision to Diff 540271.Jul 13 2023, 10:01 PM

Fix qualifiers

philnik updated this revision to Diff 540445.Jul 14 2023, 9:02 AM

Try to fix CI

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?

philnik added inline comments.Jul 14 2023, 10:12 AM
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)

Mordante added inline comments.Jul 14 2023, 10:16 AM
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.

philnik updated this revision to Diff 540556.Jul 14 2023, 1:48 PM
philnik marked 3 inline comments as done.

Address comments

philnik updated this revision to Diff 540652.Jul 15 2023, 1:42 AM

Try to fix CI

philnik updated this revision to Diff 540691.Jul 15 2023, 9:04 AM

Try to fix CI

philnik updated this revision to Diff 540702.Jul 15 2023, 10:09 AM

Try to fix CI

philnik updated this revision to Diff 540864.Jul 16 2023, 10:59 PM

Try to fix CI

Mordante accepted this revision.Jul 24 2023, 8:35 AM

LGTM, but there is a remark.

libcxx/include/module.modulemap.in
1259 ↗(On Diff #540864)

Please make sure this is rebased on the latest module map format.

This revision is now accepted and ready to land.Jul 24 2023, 8:35 AM
EricWF removed reviewers: Restricted Project, Mordante.Aug 2 2023, 2:57 PM
EricWF added a subscriber: EricWF.

@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.

Herald added a reviewer: Restricted Project. · View Herald TranscriptAug 2 2023, 2:57 PM
zibi added a subscriber: zibi.Aug 10 2023, 1:14 PM
zibi added inline comments.
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?

SeanP added a subscriber: SeanP.Aug 11 2023, 11:49 AM
SeanP added inline comments.
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?

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.