Page MenuHomePhabricator

[libc] Move implementations of cosf, sinf, sincosf to src/math directory.
Needs ReviewPublic

Authored by sivachandra on Wed, Mar 25, 10:06 PM.
This revision needs review, but there are no reviewers specified.

Details

Reviewers
None
Summary

NFC intended. Only mechanical changes to fit the LLVM libc implementation
standard have been done.

Depends on D76826.

Diff Detail

Event Timeline

sivachandra created this revision.Wed, Mar 25, 10:06 PM

This patch is not ready for review yet. For one, it does not have any tests. I am only sharing to showcase the move early.

sivachandra edited the summary of this revision. (Show Details)Wed, Mar 25, 10:09 PM
sivachandra edited the summary of this revision. (Show Details)
abrachet added inline comments.
libc/src/math/CMakeLists.txt
9

Will this properly propagate to targets which depend on math_utils?

libc/src/math/cosf.cpp
19–21

Maybe we could remove the two spaces after the period. This isn't common in the rest of our comments

libc/src/math/math_utils.h
1

Should this file have a comment header?

libc/src/math/sincosf.h
1

sinf -> sincosf

libc/src/math/sincosf_utils.h
33–36

We can put the data from sincosf_data and just make them constexpr perhaps.

This patch is not ready for review yet. For one, it does not have any tests. I am only sharing to showcase the move early.

It's probably worth discussing how much we expect we will alter these files. Is it fine to just accept that we didn't write them and that they will just be very different from the rest of libc?

It's probably worth discussing how much we expect we will alter these files. Is it fine to just accept that we didn't write them and that they will just be very different from the rest of libc?

While I do intend to make any functional changes in this move patch, we absolutely want the code to follow our implementation standards, just like the rest of the code. So, thanks for already pointing out the items missed. For now, all I have done was to run clang-format. I will fix the rest with a new upload.

It's probably worth discussing how much we expect we will alter these files. Is it fine to just accept that we didn't write them and that they will just be very different from the rest of libc?

While I do intend to make any functional changes in this move patch, we absolutely want the code to follow our implementation standards, just like the rest of the code. So, thanks for already pointing out the items missed. For now, all I have done was to run clang-format. I will fix the rest with a new upload.

I mean't to say, "do NOT intend".

Won't deleting the files in the AOR directory cause the make check on the buildbots to fail? It might make sense to leave the files and delete them once all functions have been migrated?

Won't deleting the files in the AOR directory cause the make check on the buildbots to fail? It might make sense to leave the files and delete them once all functions have been migrated?

Yes, it will. I plan to move the tests also in a way that make check does not break.