This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Fix the signature of std::rotl and std::rotr
ClosedPublic

Authored by DKay7 on Aug 9 2023, 7:10 PM.

Details

Summary
  • Changed parameters type in std::rotr and std::rorl functions from unsigned int to int.
  • Implemented behaviour for negative parameter values.

Fixes #64544

Diff Detail

Event Timeline

DKay7 created this revision.Aug 9 2023, 7:10 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 9 2023, 7:10 PM
DKay7 requested review of this revision.Aug 9 2023, 7:10 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 9 2023, 7:10 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
DKay7 edited the summary of this revision. (Show Details)Aug 9 2023, 7:11 PM
philnik requested changes to this revision.Aug 9 2023, 7:17 PM
philnik added a subscriber: philnik.

Please add tests in libcxx/test/std/numerics/bit/bitops.rot/rotr.pass.cpp for this change.

libcxx/include/__bit/rotate.h
47

We might also want to fix __rotr, depending on where it is used. Generally, the underscore versions should have the same semantics as non-underscore versions.

This revision now requires changes to proceed.Aug 9 2023, 7:17 PM
DKay7 added a comment.Aug 9 2023, 9:20 PM

We might also want to fix __rotr, depending on where it is used. Generally, the underscore versions should have the same semantics as non-underscore versions.

I implemented the behavior for negative values in __rotr and thought that now std::rotl and std::rotr could be implemented as follows:

template <__libcpp_unsigned_integer _Tp>
[[nodiscard]] _LIBCPP_HIDE_FROM_ABI constexpr _Tp rotl(_Tp __t, int __cnt) noexcept {
    return std::__rotr(__t, -__cnt);
}

template <__libcpp_unsigned_integer _Tp>
[[nodiscard]] _LIBCPP_HIDE_FROM_ABI constexpr _Tp rotr(_Tp __t, int __cnt) noexcept {
  return std::__rotr(__t, __cnt);
}

This works for positive values, because rotl is the same as rotr with a negative value and vice versa. However, it doesn't seem very obvious and might be even confusing, so I wanted to find out if it's worth doing so. Thank you.

DKay7 updated this revision to Diff 548876.Aug 9 2023, 10:12 PM
  • Added unit-tests for std::rotr and std::rorl with negative values.
  • Rewrited std::__rotr as @philnik asked.
  • Rewrited std::rotr and std::rorl via new std::__rotr, but not sure if new implementations are not confusing.
philnik added inline comments.Aug 10 2023, 10:00 AM
libcxx/include/__bit/rotate.h
40–41

It looks like your patch is missing the last commit. I'd recommend squashing the two into one and upload the resulting patch again.

DKay7 updated this revision to Diff 549126.Aug 10 2023, 11:52 AM

Updating D157569: Fixed issue #64544 ([libc++] std::rotl and std::rotr have the wrong signature)

  • Squashed two last commits into one
philnik retitled this revision from Fixed issue #64544 ([libc++] std::rotl and std::rotr have the wrong signature) to [libc++] Fix the signature of std::rotl and std::rotr.Aug 11 2023, 10:26 AM
philnik edited the summary of this revision. (Show Details)
philnik accepted this revision.Aug 11 2023, 10:29 AM

It looks like __bit/rotate.h is now formatted. Please remove it from libcxx/utils/data/ignore_format.txt.

LGTM with passing CI.

I guess you don't have commit access? In that case, please provide "Your Name" <your.email@address> for attribution.

This revision is now accepted and ready to land.Aug 11 2023, 10:29 AM
DKay7 updated this revision to Diff 549556.Aug 11 2023, 6:15 PM
DKay7 edited the summary of this revision. (Show Details)

Removed __bit/rotate.h from libcxx/utils/data/ignore_format.txt

  • My name and email (as @philnik asked): "Daniil Kalinin" kalinin.de@phystech.edu

#Updating D157569: [libc++] Fix the signature of std::rotl and std::rotr

DKay7 requested review of this revision.Aug 12 2023, 12:25 AM

AIX build has failed with "There is not enough space in the file system" error. Actually, I have no idea how to deal with it.

philnik accepted this revision.Aug 12 2023, 8:44 AM

AIX build has failed with "There is not enough space in the file system" error. Actually, I have no idea how to deal with it.

The AIX builders are currently broken. You've done nothing wrong.

This revision is now accepted and ready to land.Aug 12 2023, 8:44 AM
This revision was landed with ongoing or failed builds.Aug 12 2023, 8:46 AM
This revision was automatically updated to reflect the committed changes.