Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[libc++] Add std::fpclassify overloads for floating-point. #67913

Merged
merged 7 commits into from
Oct 5, 2023

Conversation

tolikmalibroda
Copy link
Contributor

Standard says that implementation of math functions that have floating-point-type parameter should provide an "overload for each cv-unqualified floating-point type".

@tolikmalibroda tolikmalibroda requested a review from a team as a code owner October 1, 2023 13:01
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Oct 1, 2023
Copy link
Contributor

@philnik777 philnik777 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The implementation looks good, but this needs tests. Specifically,

  • there should be a regression test to make sure an implicit conversion works now
  • please make sure that we have test coverage for the _LIBCPP_NODISCARD_EXTs
  • please make sure that we have test for the noexceptness of the overloads.

You should be able to find tests for this under libcxx/test/libcxx/{diagnostics,numerics/c.math} if they exist.

@tolikmalibroda
Copy link
Contributor Author

Sorry, I pulled commits from main :(
I'll revert it

@github-actions
Copy link

github-actions bot commented Oct 2, 2023

✅ With the latest revision this PR passed the C/C++ code formatter.

@tolikmalibroda
Copy link
Contributor Author

Now code diff's showing correct changes. But I'll probably need to create new PR to merge changes without extra commits...

Added tests for implicit conversions and noexceptness to libcxx/test/std/numerics/c.math/cmath.pass.cpp.
Tests for nodiscard already exist in libcxx/test/libcxx/diagnostics/math_nodiscard_extensions.verify.cpp.

@philnik777
Copy link
Contributor

You can just rebase to fix your issues.

@tolikmalibroda
Copy link
Contributor Author

Maybe I've done something wrong, but I've rebased and those all commits from main were added ahead of mine and appeared here...
I can somehow fix that and remove these commits from PR?

@philnik777
Copy link
Contributor

You probably did something wrong while rebasing. In a rebase workflow you want a single commit that you're force-pushing to your branch generally. It looks like you had a merge commit or something like that and the new commits got rebased onto that, resulting in different hashes; i.e. different commits. When running git log HEAD...main in your branch you should see only the commits you want to be part of this PR until you see origin/main, or upstream/main or whatever you named the upstream repo. (And no worries; I pinged dozens of people a few times too when I screwed up a rebase - happens to everyone)

@tolikmalibroda
Copy link
Contributor Author

git log HEAD...main on my branch gives all commits that I've merged (when I did rebase) and then reverted + commits relevant to this PR. I don't know if I can remove them now...

@philnik777
Copy link
Contributor

You should be able to git rebase -i main and then drop the commits.

@tolikmalibroda
Copy link
Contributor Author

Many thanks, that worked! I should definitely read more documentation on how git rebase works and be more careful next time : )

@tolikmalibroda tolikmalibroda force-pushed the fpclassify_overloads branch 2 times, most recently from a9a5c2a to 3bf5f2d Compare October 3, 2023 18:59
Standard says that implementation of math functions that have floating-point-type parameter should provide an "overload for each cv-unqualified floating-point type".
So we should provide weaker floating-point overloads.
Copy link
Contributor

@philnik777 philnik777 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with a comment added.

libcxx/include/math.h Show resolved Hide resolved
@philnik777 philnik777 merged commit dc129d6 into llvm:main Oct 5, 2023
2 checks passed
@tolikmalibroda tolikmalibroda deleted the fpclassify_overloads branch October 5, 2023 19:27
Guzhu-AMD pushed a commit to GPUOpen-Drivers/llvm-project that referenced this pull request Oct 12, 2023
Local branch amd-gfx bedba19 Merged main:ea2036e1e56b into amd-gfx:e1b9ddbd68c9
Remote branch main dc129d6 [libc++] Add std::fpclassify overloads for floating-point. (llvm#67913)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants