-
Notifications
You must be signed in to change notification settings - Fork 11.9k
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
Changes from all commits
bd82810
1aa5e01
cf0e266
dd7e838
8e475a5
99900ba
51db352
9423973
be73bf4
a6c9452
4a124c9
f79cd96
6ceb5c7
764a47d
48132c9
42dfd37
1cacd71
1689a30
0e462fc
1888965
ed76b82
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This change seems like an ABI break, can you add There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The type of the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ldionne are you happy with this change without using Basically the change is a a different type and the signature There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 I do agree this is technically an ABI break due to the change from |
||
# else | ||
constexpr __ignore_type ignore; | ||
# endif | ||
|
||
_LIBCPP_END_NAMESPACE_STD | ||
|
||
#endif // _LIBCPP_CXX03_LANG | ||
|
||
#endif // _LIBCPP___TUPLE_IGNORE_H |
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> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you add a comment along the lines of
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The paper also "recommends" to mention the same in
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 The wording section contains
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; |
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; |
There was a problem hiding this comment.
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).There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
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)?
There was a problem hiding this comment.
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 toinline
only since C++17._INLINE_VAR
expands to nothing in C++14 mode, which makesconstexpr
variables non-inline and have internal linkage. I think libc++ is effectively doing the same.