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++][tuple][utility] P2968R2: Make std::ignore a first-class object #97401

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
bd82810
[libc++][tuple][utility] P2968R2 Make `std::ignore` a first-class object
H-G-Hristov Jul 2, 2024
1aa5e01
Fixed C++11
H-G-Hristov Jul 3, 2024
cf0e266
Updated test
H-G-Hristov Jul 3, 2024
dd7e838
Formatting
H-G-Hristov Jul 3, 2024
8e475a5
Merge branch 'main' into hgh/libcxx/P2968R2-Make-std-ignore-a-first-c…
Zingam Jul 3, 2024
99900ba
Updated tests
H-G-Hristov Jul 5, 2024
51db352
Merge branch 'hgh/libcxx/P2968R2-Make-std-ignore-a-first-class-object…
H-G-Hristov Jul 5, 2024
9423973
Merge branch 'main' into hgh/libcxx/P2968R2-Make-std-ignore-a-first-c…
H-G-Hristov Jul 5, 2024
be73bf4
Added `include<>` tests
H-G-Hristov Jul 6, 2024
a6c9452
Merge branch 'main' into hgh/libcxx/P2968R2-Make-std-ignore-a-first-c…
H-G-Hristov Jul 7, 2024
4a124c9
Addressed comments
H-G-Hristov Jul 7, 2024
f79cd96
Fixes
H-G-Hristov Jul 7, 2024
6ceb5c7
More fixes
H-G-Hristov Jul 7, 2024
764a47d
Merge branch 'main' into hgh/libcxx/P2968R2-Make-std-ignore-a-first-c…
H-G-Hristov Jul 7, 2024
48132c9
Fixes
H-G-Hristov Jul 7, 2024
42dfd37
Merge branch 'hgh/libcxx/P2968R2-Make-std-ignore-a-first-class-object…
H-G-Hristov Jul 7, 2024
1cacd71
Addressed review comments
H-G-Hristov Jul 7, 2024
1689a30
Fixed a typo
H-G-Hristov Jul 7, 2024
0e462fc
Merge branch 'main' into hgh/libcxx/P2968R2-Make-std-ignore-a-first-c…
H-G-Hristov Jul 9, 2024
1888965
Updated status pages
H-G-Hristov Jul 9, 2024
ed76b82
Merge branch 'main' into hgh/libcxx/P2968R2-Make-std-ignore-a-first-c…
H-G-Hristov Jul 9, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions libcxx/docs/ReleaseNotes/19.rst
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ Implemented Papers
- P2872R3 - Remove ``wstring_convert`` From C++26
- P3142R0 - Printing Blank Lines with ``println`` (as DR against C++23)
- P2944R3 - Comparisons for ``reference_wrapper`` (comparison operators for ``reference_wrapper`` only)
- P2968R2 - Make ``std::ignore`` a first-class object
- P2302R4 - ``std::ranges::contains``
- P1659R3 - ``std::ranges::starts_with`` and ``std::ranges::ends_with``
- P3029R1 - Better ``mdspan``'s CTAD
Expand Down Expand Up @@ -74,6 +75,9 @@ Improvements and New Features

- The formatting library is updated to Unicode 15.1.0.

- ``std::ignore``\s ``const __ignore_t& operator=(_Tp&&) const`` was changed to
``const __ignore_type& operator=(const _Tp&) const noexcept`` for all language versions.

Deprecations and Removals
-------------------------

Expand Down
2 changes: 1 addition & 1 deletion libcxx/docs/Status/Cxx2cPapers.csv
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@
"`P2985R0 <https://wg21.link/P2985R0>`__","LWG","A type trait for detecting virtual base classes","St. Louis June 2024","","",""
"`P0843R14 <https://wg21.link/P0843R14>`__","LWG","``inplace_vector``","St. Louis June 2024","","",""
"`P3235R3 <https://wg21.link/P3235R3>`__","LWG","``std::print`` more types faster with less memory","St. Louis June 2024","","","|format| |DR|"
"`P2968R2 <https://wg21.link/P2968R2>`__","LWG","Make ``std::ignore`` a first-class object","St. Louis June 2024","","",""
"`P2968R2 <https://wg21.link/P2968R2>`__","LWG","Make ``std::ignore`` a first-class object","St. Louis June 2024","|Complete|","19.0",""
"`P2075R6 <https://wg21.link/P2075R6>`__","LWG","Philox as an extension of the C++ RNG engines","St. Louis June 2024","","",""
"`P2422R1 <https://wg21.link/P2422R1>`__","LWG","Remove ``nodiscard`` annotations from the standard library specification","St. Louis June 2024","|Complete| [#note-P2422R1]_","19.0",""
"`P2300R10 <https://wg21.link/P2300R10>`__","LWG","``std::execution``","St. Louis June 2024","","",""
Expand Down
1 change: 1 addition & 0 deletions libcxx/include/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -710,6 +710,7 @@ set(files
__thread/timed_backoff_policy.h
__tree
__tuple/find_index.h
__tuple/ignore.h
__tuple/make_tuple_types.h
__tuple/sfinae_helpers.h
__tuple/tuple_element.h
Expand Down
39 changes: 39 additions & 0 deletions libcxx/include/__tuple/ignore.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
//===----------------------------------------------------------------------===//
//
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
// See https://llvm.org/LICENSE.txt for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
//
//===----------------------------------------------------------------------===//

#ifndef _LIBCPP___TUPLE_IGNORE_H
#define _LIBCPP___TUPLE_IGNORE_H

#include <__config>

#if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
# pragma GCC system_header
#endif

#ifndef _LIBCPP_CXX03_LANG

_LIBCPP_BEGIN_NAMESPACE_STD

struct __ignore_type {
template <class _Tp>
_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14 const __ignore_type& operator=(const _Tp&) const noexcept {
return *this;
}
};

# if _LIBCPP_STD_VER >= 17
inline constexpr __ignore_type ignore;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is backporting a change in C++17 (P0607R0)... I think in C++11/14 std::ignore was supported to have internal linkage (and thus denoted different objects in different TUs).

Copy link
Contributor

Choose a reason for hiding this comment

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

This is backporting a change in C++17 (P0607R0)... I think in C++11/14 std::ignore was supported to have internal linkage (and thus denoted different objects in different TUs).

What's the benefit of doing this?

Copy link
Contributor

Choose a reason for hiding this comment

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

What's the benefit of doing this?

This is what libc++ is doing currently (since 8643bdd). I guess it would be better to backport inline additions in a concentrated PR if wanted.

Copy link
Contributor Author

@H-G-Hristov H-G-Hristov Jul 7, 2024

Choose a reason for hiding this comment

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

@frederick-vs-ja Thanks for the review. Isn't the current implementation in STL in line with what's I implemented here (also regarding your comment above)?

Copy link
Contributor

Choose a reason for hiding this comment

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

MSVC STL is currently marking these non-templated constexpr variables _INLINE_VAR, which expands to inline only since C++17. _INLINE_VAR expands to nothing in C++14 mode, which makes constexpr variables non-inline and have internal linkage. I think libc++ is effectively doing the same.

Copy link
Member

Choose a reason for hiding this comment

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

This change seems like an ABI break, can you add _LIBCPP_HIDE_FROM_ABI?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure what ABI break you see, but this definitely shouldn't be makred _LIBCPP_HIDE_FROM_ABI, since that would be non-conforming. You would get different addresses in different dylibs.

Copy link
Member

Choose a reason for hiding this comment

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

The type of the ignore changes and that type is not identical. Why would adding _LIBCPP_HIDE_FROM_ABI be non-conforming?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure it's technically an ODR violation, but that's benign AFAICT. We don't change the layout in any way. As I already said, it's non-conforming because the object would have different addresses in different dylibs.

Copy link
Member

Choose a reason for hiding this comment

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

@ldionne are you happy with this change without using _LIBCPP_HIDE_FROM_ABI.

Basically the change is a a different type and the signature operator=(_Tp&&) const changed to operator=(const _Tp&) const noexcept

Copy link
Member

Choose a reason for hiding this comment

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

The current way it is defined seems correct to me. We do need to ensure that ignore has the same address across all TUs since the Standard says inline constexpr explicitly, so we can't ABI-tag this.

I do agree this is technically an ABI break due to the change from __ignore_t<unsigned char> to __ignore_type, however I really really don't think it matters in practice. I don't even feel it is worth release-noting this ABI change since I think essentially nobody would ever notice.

# else
constexpr __ignore_type ignore;
# endif

_LIBCPP_END_NAMESPACE_STD

#endif // _LIBCPP_CXX03_LANG

#endif // _LIBCPP___TUPLE_IGNORE_H
1 change: 1 addition & 0 deletions libcxx/include/module.modulemap
Original file line number Diff line number Diff line change
Expand Up @@ -1840,6 +1840,7 @@ module std_private_thread_thread [system] {
module std_private_thread_timed_backoff_policy [system] { header "__thread/timed_backoff_policy.h" }

module std_private_tuple_find_index [system] { header "__tuple/find_index.h" }
module std_private_tuple_ignore [system] { header "__tuple/ignore.h" }
module std_private_tuple_make_tuple_types [system] { header "__tuple/make_tuple_types.h" }
module std_private_tuple_tuple_like_no_subrange [system] {
header "__tuple/tuple_like_no_subrange.h"
Expand Down
24 changes: 7 additions & 17 deletions libcxx/include/tuple
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,12 @@ tuple(allocator_arg_t, Alloc, pair<T1, T2>) -> tuple<T1, T2>; // since C++
template <class Alloc, class ...T>
tuple(allocator_arg_t, Alloc, tuple<T...>) -> tuple<T...>; // since C++17

inline constexpr unspecified ignore;
struct ignore-type { // exposition only // Since C++26
constexpr const ignore-type&
operator=(const auto &) const noexcept
{ return *this; }
};
inline constexpr ignore-type ignore;

template <class... T> tuple<V...> make_tuple(T&&...); // constexpr in C++14
template <class... T> tuple<ATypes...> forward_as_tuple(T&&...) noexcept; // constexpr in C++14
Expand Down Expand Up @@ -215,6 +220,7 @@ template <class... Types>
#include <__memory/allocator_arg_t.h>
#include <__memory/uses_allocator.h>
#include <__tuple/find_index.h>
#include <__tuple/ignore.h>
#include <__tuple/make_tuple_types.h>
#include <__tuple/sfinae_helpers.h>
#include <__tuple/tuple_element.h>
Expand Down Expand Up @@ -1112,22 +1118,6 @@ inline _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14 tuple<_Tp&...> tie(_T
return tuple<_Tp&...>(__t...);
}

template <class _Up>
struct __ignore_t {
template <class _Tp>
_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14 const __ignore_t& operator=(_Tp&&) const {
return *this;
}
};

# if _LIBCPP_STD_VER >= 17
inline constexpr __ignore_t<unsigned char> ignore = __ignore_t<unsigned char>();
# else
namespace {
constexpr __ignore_t<unsigned char> ignore = __ignore_t<unsigned char>();
} // namespace
# endif

template <class... _Tp>
inline _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14 tuple<typename __unwrap_ref_decay<_Tp>::type...>
make_tuple(_Tp&&... __t) {
Expand Down
4 changes: 4 additions & 0 deletions libcxx/include/utility
Original file line number Diff line number Diff line change
Expand Up @@ -274,6 +274,10 @@ template <class T>
#include <compare>
#include <initializer_list>

// [tuple.creation]

#include <__tuple/ignore.h>
mordante marked this conversation as resolved.
Show resolved Hide resolved

// [tuple.helper]
#include <__tuple/tuple_element.h>
#include <__tuple/tuple_size.h>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
//===----------------------------------------------------------------------===//
//
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
// See https://llvm.org/LICENSE.txt for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
//
//===----------------------------------------------------------------------===//

// UNSUPPORTED: c++03

// <tuple>
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a comment along the lines of

std::ignore should be provided by the headers <tuple> and <utility>.
This tests validates its presence in <tuple>.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The paper also "recommends" to mention the same in <tuple>. Do you have any recommendations what and where to write?

YES, make ignore available through , and not repeat the specification, but follow the example of ranges accessor functions by saying in [tuple] somewhere that ignore is available also from header .

Copy link
Member

Choose a reason for hiding this comment

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

I'm not entirely sure what you mean with "The paper also "recommends" to mention the same in <tuple>"

The wording section contains

In [tuple.general](https://eel.is/c++draft/tuple.general) append the following paragraph

<ins>In addition to being available via inclusion of the <tuple> header, ignore ([tuple.syn]) is available when <utility> ([utility]) is included.</ins>

This is not a recommendation but a requirement.

Can you explain what your question is? Feel free to do it on Discord if you prefer that.


// inline constexpr ignore-type ignore;

// std::ignore should be provided by the headers <tuple> and <utility>.
// This test validates its presence in <tuple>.

#include <tuple>

[[maybe_unused]] auto& ignore_v = std::ignore;
77 changes: 45 additions & 32 deletions libcxx/test/std/utilities/tuple/tuple.general/ignore.pass.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -6,51 +6,64 @@
//
//===----------------------------------------------------------------------===//

// <tuple>
// UNSUPPORTED: c++03

// constexpr unspecified ignore;
// <tuple>

// UNSUPPORTED: c++03
// inline constexpr ignore-type ignore;

#include <cassert>
#include <cstdint>
#include <tuple>
#include <type_traits>

#include "test_macros.h"

constexpr bool test_ignore_constexpr()
{
#if TEST_STD_VER > 11
{ // Test that std::ignore provides constexpr converting assignment.
auto& res = (std::ignore = 42);
assert(&res == &std::ignore);
}
{ // Test that std::ignore provides constexpr copy/move constructors
auto copy = std::ignore;
auto moved = std::move(copy);
((void)moved);
}
{ // Test that std::ignore provides constexpr copy/move assignment
auto copy = std::ignore;
copy = std::ignore;
auto moved = std::ignore;
moved = std::move(copy);
}
static_assert(std::is_trivial<decltype(std::ignore)>::value, "");

#if TEST_STD_VER >= 17
[[nodiscard]] constexpr int test_nodiscard() { return 8294; }
#endif
return true;

TEST_CONSTEXPR_CXX14 bool test() {
{ [[maybe_unused]] auto& ignore_v = std::ignore; }

{ // Test that std::ignore provides converting assignment.
auto& res = (std::ignore = 42);
static_assert(noexcept(res = (std::ignore = 42)), "Must be noexcept");
assert(&res == &std::ignore);
}
{ // Test bit-field binding.
mordante marked this conversation as resolved.
Show resolved Hide resolved
struct S {
unsigned int bf : 3;
};
S s{0b010};
auto& res = (std::ignore = s.bf);
assert(&res == &std::ignore);
}
{ // Test that std::ignore provides copy/move constructors
auto copy = std::ignore;
[[maybe_unused]] auto moved = std::move(copy);
}
{ // Test that std::ignore provides copy/move assignment
auto copy = std::ignore;
copy = std::ignore;
auto moved = std::ignore;
moved = std::move(copy);
}

#if TEST_STD_VER >= 17
{ std::ignore = test_nodiscard(); }
#endif

return true;
}

int main(int, char**) {
{
constexpr auto& ignore_v = std::ignore;
((void)ignore_v);
}
{
static_assert(test_ignore_constexpr(), "");
}
{
LIBCPP_STATIC_ASSERT(std::is_trivial<decltype(std::ignore)>::value, "");
}
test();
#if TEST_STD_VER >= 14
static_assert(test(), "");
#endif

return 0;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
//===----------------------------------------------------------------------===//
//
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
// See https://llvm.org/LICENSE.txt for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
//
//===----------------------------------------------------------------------===//

// UNSUPPORTED: c++03

// <utility>

// inline constexpr ignore-type ignore;

// std::ignore should be provided by the headers <tuple> and <utility>.
// This test validates its presence in <utility>.

#include <utility>

[[maybe_unused]] auto& ignore_v = std::ignore;