Skip to content

[libc++] Implements LWG3953 #107535

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

Merged
merged 2 commits into from
Sep 9, 2024
Merged

[libc++] Implements LWG3953 #107535

merged 2 commits into from
Sep 9, 2024

Conversation

NoumanAmir657
Copy link
Contributor

@NoumanAmir657 NoumanAmir657 commented Sep 6, 2024

Closes #105303

@NoumanAmir657 NoumanAmir657 requested a review from a team as a code owner September 6, 2024 07:35
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Sep 6, 2024
@llvmbot
Copy link
Member

llvmbot commented Sep 6, 2024

@llvm/pr-subscribers-libcxx

Author: NoumanAmir-10xe (NoumanAmir657)

Changes

For the issue: #105303
LWG3953


Full diff: https://github.com/llvm/llvm-project/pull/107535.diff

2 Files Affected:

  • (modified) libcxx/include/__iterator/common_iterator.h (+1-1)
  • (modified) libcxx/include/__iterator/counted_iterator.h (+1-1)
diff --git a/libcxx/include/__iterator/common_iterator.h b/libcxx/include/__iterator/common_iterator.h
index 199de2cc7337b0..ff382f32fe8950 100644
--- a/libcxx/include/__iterator/common_iterator.h
+++ b/libcxx/include/__iterator/common_iterator.h
@@ -235,7 +235,7 @@ class common_iterator {
     return std::__unchecked_get<_Sent>(__x.__hold_) - std::__unchecked_get<_I2>(__y.__hold_);
   }
 
-  _LIBCPP_HIDE_FROM_ABI friend constexpr iter_rvalue_reference_t<_Iter>
+  _LIBCPP_HIDE_FROM_ABI friend constexpr decltype(auto)
   iter_move(const common_iterator& __i) noexcept(noexcept(ranges::iter_move(std::declval<const _Iter&>())))
     requires input_iterator<_Iter>
   {
diff --git a/libcxx/include/__iterator/counted_iterator.h b/libcxx/include/__iterator/counted_iterator.h
index ea2832e3b978dc..a02c61bf34e2f9 100644
--- a/libcxx/include/__iterator/counted_iterator.h
+++ b/libcxx/include/__iterator/counted_iterator.h
@@ -249,7 +249,7 @@ class counted_iterator
     return __rhs.__count_ <=> __lhs.__count_;
   }
 
-  _LIBCPP_HIDE_FROM_ABI friend constexpr iter_rvalue_reference_t<_Iter>
+  _LIBCPP_HIDE_FROM_ABI friend constexpr decltype(auto)
   iter_move(const counted_iterator& __i) noexcept(noexcept(ranges::iter_move(__i.__current_)))
     requires input_iterator<_Iter>
   {

@NoumanAmir657 NoumanAmir657 changed the title Update return type of iter_move in common_iterator and counted_iterator [libc++] Update return type of iter_move in common_iterator and counted_iterator Sep 6, 2024
@frederick-vs-ja
Copy link
Contributor

For the issue:

It would be better to use GitHub's close/fix/resolve syntax in the PR description to associate this PR with the issue.

Also, it's better to explicitly say LWG3953 in the title of this PR. Note that the actual return types remain unchanged.

@NoumanAmir657 NoumanAmir657 changed the title [libc++] Update return type of iter_move in common_iterator and counted_iterator [libc++] LWG3953 Sep 6, 2024
@NoumanAmir657
Copy link
Contributor Author

For the issue:

It would be better to use GitHub's close/fix/resolve syntax in the PR description to associate this PR with the issue.

Also, it's better to explicitly say LWG3953 in the title of this PR. Note that the actual return types remain unchanged.

Fixed.

Copy link
Contributor

@frederick-vs-ja frederick-vs-ja left a comment

Choose a reason for hiding this comment

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

No change requested. The modification is almost, if not exactly, NFC, given input_iterator has already required that iter_rvalue_reference_t is valid.

It would be extremely weird to me to test something (if exists) for which iter_rvalue_reference_t results in substitution failure while input_iterator gives hard error (caused by some stuff checked before iter_rvalue_reference_t).

@Zingam
Copy link
Contributor

Zingam commented Sep 6, 2024

You still need to update the cvs file I think:

"`LWG3953 <https://wg21.link/LWG3953>`__","``iter_move`` for ``common_iterator`` and ``counted_iterator`` should return ``decltype(auto)``","2023-11 (Kona)","","",""

@NoumanAmir657 NoumanAmir657 changed the title [libc++] LWG3953 [libc++] Implements LWG3953 Sep 6, 2024
@NoumanAmir657
Copy link
Contributor Author

You still need to update the cvs file I think:

"`LWG3953 <https://wg21.link/LWG3953>`__","``iter_move`` for ``common_iterator`` and ``counted_iterator`` should return ``decltype(auto)``","2023-11 (Kona)","","",""

Updated. Thanks!

@NoumanAmir657
Copy link
Contributor Author

Any idea why the CI failed? @frederick-vs-ja @Zingam .

@Zingam
Copy link
Contributor

Zingam commented Sep 7, 2024

Any idea why the CI failed? @frederick-vs-ja @Zingam .

I restarted the failed CI job. Let's wait to see if it will pass.

@NoumanAmir657
Copy link
Contributor Author

Any idea why the CI failed? @frederick-vs-ja @Zingam .

I restarted the failed CI job. Let's wait an see if it will pass.

Aha okay. Thanks.

@NoumanAmir657
Copy link
Contributor Author

Any idea why the CI failed? @frederick-vs-ja @Zingam .

I restarted the failed CI job. Let's wait to see if it will pass.

Yeah seems like it passed now.

@ldionne ldionne merged commit 6776d65 into llvm:main Sep 9, 2024
62 checks passed
@ldionne
Copy link
Member

ldionne commented Sep 9, 2024

@NoumanAmir657 Thanks for the patch! Please turn off the "hide my email" feature of Github for your future contributions since we require a valid email address for LLVM contributions.

@NoumanAmir657 NoumanAmir657 deleted the iter_move branch September 9, 2024 19:18
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.

LWG3953: iter_move for common_iterator and counted_iterator should return decltype(auto)
5 participants