[libcxx-commits] [PATCH] D151637: DRAFT: hardening "interface"
Louis Dionne via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Thu Jun 22 10:23:37 PDT 2023
var-const created this revision.
Herald added a project: All.
var-const updated this revision to Diff 526366.
var-const added a comment.
var-const marked an inline comment as done.
var-const marked 3 inline comments as done.
var-const updated this revision to Diff 533505.
var-const marked 6 inline comments as done.
var-const updated this revision to Diff 533506.
var-const updated this revision to Diff 533507.
ldionne published this revision for review.
Herald added a project: libc++.
Herald added a subscriber: libcxx-commits.
Herald added a reviewer: libc++.
Undo accidental change.
var-const added a comment.
Address more feedback.
var-const added a comment.
__hardening.h -> __hardening
var-const added a comment.
Quick fix.
ldionne added a comment.
I really like the shape of this. At a high level, here's what I would recommend:
1. Extract the ABI macro for bounded iterators out of this patch. This will make this patch smaller and also get that out of the way when you try to remove the legacy debug mode.
2. Introduce the assertions machinery in a separate patch: `_LIBCPP_ENABLE_HARDENED_MODE` and `_LIBCPP_ENABLE_NEW_DEBUG_MODE`, but without any of the check macros like `_LIBCPP_ASSERTIONS_ENABLE_CHECK_BAD_CONTAINER_ACCESS`. Yes, that patch is going to look kinda stupid, but that will allow us to properly consider the interaction between those settings and the existing settings we have.
3. Then, in a separate patch, let's introduce some (or all) of the `_LIBCPP_ASSERTIONS_ENABLE_CHECK_foo` checks and plumb them into `_LIBCPP_ENABLE_HARDENED_MODE`/`_LIBCPP_ENABLE_NEW_DEBUG_MODE` which will already have been introduced.
================
Comment at: libcxx/include/__config:202-204
+// Enables all the checks that break the ABI.
+//
+// #define _LIBCPP_ENABLE_HARDENED_MODE_ABI_BREAK 1
----------------
I would suggest not introducing this macro, at least not for now. Instead, I would rename `_LIBCPP_ASSERTIONS_ENABLE_HARDENING_BOUNDED_ITERATORS` to `_LIBCPP_ABI_BOUNDED_ITERATORS`, a normal ABI macro like we have others and I would simply not define it in any configuration by default. In fact, this could be done in a separate patch to simplify this one quite a bit.
Actually if you do that in a separate patch, `_LIBCPP_ABI_BOUNDED_ITERATORS` could take over `_LIBCPP_DEBUG_ITERATOR_BOUNDS_CHECKING`.
================
Comment at: libcxx/include/__config:248
+//
+// #define _LIBCPP_ASSERTIONS_ENABLE_HARDENING_ASSERTIONS 1
+
----------------
Since this affects both hardening and "debug" checks, I don't think `_LIBCPP_ASSERTIONS_ENABLE_HARDENING_ASSERTIONS` is the right name. Is this different from `_LIBCPP_ENABLE_ASSERTIONS`?
Answer: The difference between this macro and `_LIBCPP_ENABLE_ASSERTIONS` is that the latter also disables some currently-uncategorized assertions.
IMO it's not worth introducing another public knob for this, I think people don't need that much granularity. I can't think of anyone wanting to disable hardening and debug assertions while somehow keeping all the "uncategorized" assertions.
================
Comment at: libcxx/include/__config:280-282
+# if _LIBCPP_ENABLE_NEW_DEBUG_MODE
+# error "Only one of _LIBCPP_ENABLE_HARDENED_MODE and _LIBCPP_ENABLE_NEW_DEBUG_MODE can be defined."
+# endif
----------------
Suggestion: instead, you could do this outside of the `#if` block:
```
#if _LIBCPP_ENABLE_HARDENED_MODE && _LIBCPP_ENABLE_NEW_DEBUG_MODE
# error "Only one of _LIBCPP_ENABLE_HARDENED_MODE and _LIBCPP_ENABLE_NEW_DEBUG_MODE can be defined."
#endif
```
================
Comment at: libcxx/include/__config:322
+# define _LIBCPP_ASSERT_CHECK_INVALID_RANGE2(range, message) \
+ _LIBCPP_HARDENING_ASSERT(__hardening::_CheckInvalidRange(range), message)
+# define _LIBCPP_ASSERT_CHECK_INVALID_RANGE3(first, last, message) \
----------------
Let's fully qualify everything in macros.
================
Comment at: libcxx/include/__config:88
+/*
+#define _LIBCPP_ENABLE_ASSERTIONS 1
+#define _LIBCPP_ENABLE_HARDENED_MODE 1
----------------
These are leftovers from testing, but I'm keeping them for now as useful reminders.
================
Comment at: libcxx/include/__config:92
+#define _LIBCPP_ENABLE_NEW_DEBUG_MODE 0
+#define _LIBCPP_ASSERTIONS_ENABLE_FEATURE_RANDOMIZE_UNSPECIFIED_BEHAVIOR 0
+*/
----------------
Note to self: this prevents a compiler warning/error.
================
Comment at: libcxx/include/__config:292
+
+# define _LIBCPP_ASSERTIONS_ENABLE_HARDENING_ASSERTIONS 1
+
----------------
IIUC, the current state where `_LIBCPP_ENABLE_NEW_DEBUG_MODE` automatically enables `_LIBCPP_ASSERTIONS_ENABLE_HARDENING_ASSERTIONS` doesn't quite work because then the user cannot use `_LIBCPP_ASSERTIONS_ENABLE_HARDENING_ASSERTIONS` to override the current hardening mode, right? If so, does that mean that the user has to define both e.g. `_LIBCPP_ASSERTIONS_ENABLE_HARDENING_ASSERTIONS` and `_LIBCPP_ENABLE_NEW_DEBUG_MODE`?
================
Comment at: libcxx/include/__config:331
+
+# define _LIBCPP_ASSERT_CHECK_INVALID_RANGE(...) ((void)0)
+
----------------
I copied this from existing macros. Is there a reason to prefer this form to just `/*nothing*/`?
================
Comment at: libcxx/include/__config:331
+
+# define _LIBCPP_ASSERT_CHECK_INVALID_RANGE(...) ((void)0)
+
----------------
var-const wrote:
> I copied this from existing macros. Is there a reason to prefer this form to just `/*nothing*/`?
I think it's to make this valid to use in an expression. I have a test that `_LIBCPP_ASSERT` can be used in an expression, and we should have similar tests for new assertion macros being introduced (possibly add to the same test).
================
Comment at: libcxx/include/__config_hardening:20-40
+// Available ABI-breaking checks:
+
+// Changes the iterator type of select containers (see below) to a bounded iterator that:
+// - keeps track of which container it refers to;
+// - keeps track of whether it's valid or not and ASSERTS validity on every dereference;
+// - allows enabling the check that a given range is valid (sentinel is reachable from the begin iterator, etc.) in
+// constant time.
----------------
This should move to `__config` under the ABI selection block. Or while we're at it, I've been meaning to move the ABI checks to somewhere outside of `__config` for a while but never got to it, so this could be a refactoring we do before adding these new knobs.
Maybe `__config_dir/abi.h`, `__config_dir/assertions.h`, `__config_dir/availability.h`, etc?
================
Comment at: libcxx/include/__config_hardening:20-40
+// Available ABI-breaking checks:
+
+// Changes the iterator type of select containers (see below) to a bounded iterator that:
+// - keeps track of which container it refers to;
+// - keeps track of whether it's valid or not and ASSERTS validity on every dereference;
+// - allows enabling the check that a given range is valid (sentinel is reachable from the begin iterator, etc.) in
+// constant time.
----------------
ldionne wrote:
> This should move to `__config` under the ABI selection block. Or while we're at it, I've been meaning to move the ABI checks to somewhere outside of `__config` for a while but never got to it, so this could be a refactoring we do before adding these new knobs.
>
> Maybe `__config_dir/abi.h`, `__config_dir/assertions.h`, `__config_dir/availability.h`, etc?
Created https://reviews.llvm.org/D151900 to move things under `__config_dir` (without any other changes for now). We want to move `__config` itself into the new directory as well, right?
================
Comment at: libcxx/include/__config_hardening:20-40
+// Available ABI-breaking checks:
+
+// Changes the iterator type of select containers (see below) to a bounded iterator that:
+// - keeps track of which container it refers to;
+// - keeps track of whether it's valid or not and ASSERTS validity on every dereference;
+// - allows enabling the check that a given range is valid (sentinel is reachable from the begin iterator, etc.) in
+// constant time.
----------------
var-const wrote:
> ldionne wrote:
> > This should move to `__config` under the ABI selection block. Or while we're at it, I've been meaning to move the ABI checks to somewhere outside of `__config` for a while but never got to it, so this could be a refactoring we do before adding these new knobs.
> >
> > Maybe `__config_dir/abi.h`, `__config_dir/assertions.h`, `__config_dir/availability.h`, etc?
> Created https://reviews.llvm.org/D151900 to move things under `__config_dir` (without any other changes for now). We want to move `__config` itself into the new directory as well, right?
Update: this turned out to be a lot harder than initially assumed because the split makes it easier to forget to include a header and mistakenly presume that a macro is undefined. I will postpone D151900 for now and keep everything in the same `__config` file for the purposes of this patch.
Moved to the ABI section of `__config` instead.
================
Comment at: libcxx/include/__config_hardening:61
+//#define _LIBCPP_ASSERTIONS_ENABLE_DEBUG_RANDOMIZE_UNSPECIFIED_BEHAVIOR
+//#define _LIBCPP_ASSERTIONS_ENABLE_DEBUG_CHECK_UNSORTED_ETC_INPUT
+
----------------
Let's create a bug report on github to track this improvement and explain what it is in a few words.
================
Comment at: libcxx/include/__config_hardening:61
+//#define _LIBCPP_ASSERTIONS_ENABLE_DEBUG_RANDOMIZE_UNSPECIFIED_BEHAVIOR
+//#define _LIBCPP_ASSERTIONS_ENABLE_DEBUG_CHECK_UNSORTED_ETC_INPUT
+
----------------
ldionne wrote:
> Let's create a bug report on github to track this improvement and explain what it is in a few words.
https://github.com/llvm/llvm-project/issues/63153
================
Comment at: libcxx/include/__config_hardening:63
+
+#if defined(_LIBCPP_ENABLE_HARDENING_MODE)
+
----------------
We should have a way to define default values for those in the `__config_site` so that a vendor can pick a default value for a given platform. This will probably require using `0` and `1` instead of just `#ifndef` to check for these macros.
================
Comment at: libcxx/include/__config_hardening:63
+
+#if defined(_LIBCPP_ENABLE_HARDENING_MODE)
+
----------------
ldionne wrote:
> We should have a way to define default values for those in the `__config_site` so that a vendor can pick a default value for a given platform. This will probably require using `0` and `1` instead of just `#ifndef` to check for these macros.
We should add documentation for these macros where we document the safe libc++ mode (actually this needs more updating in light of the "new" debug mode).
================
Comment at: libcxx/include/__config_hardening:63
+
+#if defined(_LIBCPP_ENABLE_HARDENING_MODE)
+
----------------
ldionne wrote:
> ldionne wrote:
> > We should have a way to define default values for those in the `__config_site` so that a vendor can pick a default value for a given platform. This will probably require using `0` and `1` instead of just `#ifndef` to check for these macros.
> We should add documentation for these macros where we document the safe libc++ mode (actually this needs more updating in light of the "new" debug mode).
We should have tests that a user can enable/disable the mode on a per-TU basis -- that's a pretty important property of the design. You can probably take inspiration from `libcxx/test/libcxx/assertions/assertions_disabled.pass.cpp`.
================
Comment at: libcxx/include/__config_hardening:63
+
+#if defined(_LIBCPP_ENABLE_HARDENING_MODE)
+
----------------
ldionne wrote:
> ldionne wrote:
> > ldionne wrote:
> > > We should have a way to define default values for those in the `__config_site` so that a vendor can pick a default value for a given platform. This will probably require using `0` and `1` instead of just `#ifndef` to check for these macros.
> > We should add documentation for these macros where we document the safe libc++ mode (actually this needs more updating in light of the "new" debug mode).
> We should have tests that a user can enable/disable the mode on a per-TU basis -- that's a pretty important property of the design. You can probably take inspiration from `libcxx/test/libcxx/assertions/assertions_disabled.pass.cpp`.
It looks like the flags I'm passing to the command line get ignored (probably overwritten), I'll need to investigate more to find out why.
================
Comment at: libcxx/include/__config_hardening:71
+
+#elif defined(_LIBCPP_ENABLE_DEBUG_MODE)
+
----------------
This is going to conflict with the existing "legacy" debug mode. Can you write a short RFC proposing the removal of the legacy debug mode since it's really causing trouble here and that legacy debug mode has been broken. A few problems about it:
1. It takes a global lock
2. It's not constexpr friendly
3. It hasn't really been maintained (e.g. new features don't necessarily use it)
4. It was design with the goal of not changing the ABI, but that didn't work because updating the global database is an ABI affecting property (for example if you get a `std::string` from the dylib which hasn't been built with the debug mode enabled, it won't be in the database so you'll fail if your user code tries to check that the `std::string` is in the database).
5. Some vendors (at least Apple) never shipped it
================
Comment at: libcxx/include/__config_hardening:71
+
+#elif defined(_LIBCPP_ENABLE_DEBUG_MODE)
+
----------------
ldionne wrote:
> This is going to conflict with the existing "legacy" debug mode. Can you write a short RFC proposing the removal of the legacy debug mode since it's really causing trouble here and that legacy debug mode has been broken. A few problems about it:
> 1. It takes a global lock
> 2. It's not constexpr friendly
> 3. It hasn't really been maintained (e.g. new features don't necessarily use it)
> 4. It was design with the goal of not changing the ABI, but that didn't work because updating the global database is an ABI affecting property (for example if you get a `std::string` from the dylib which hasn't been built with the debug mode enabled, it won't be in the database so you'll fail if your user code tries to check that the `std::string` is in the database).
> 5. Some vendors (at least Apple) never shipped it
Posted the RFC to Discourse: https://discourse.llvm.org/t/rfc-removing-the-legacy-debug-mode-from-libc/71026/1
================
Comment at: libcxx/include/__config_hardening:78
+// TODO:
+//# define _LIBCPP_ASSERTIONS_ENABLE_DEBUG_RANDOMIZE_UNSPECIFIED_BEHAVIOR
+//# define _LIBCPP_ASSERTIONS_ENABLE_DEBUG_CHECK_UNSORTED_ETC_INPUT
----------------
For this one, we probably want to do
```
// in __config_dir/assertions.h
#ifdef _LIBCPP_DEBUG_RANDOMIZE_UNSPECIFIED_STABILITY
# define _LIBCPP_ASSERTIONS_ENABLE_DEBUG_RANDOMIZE_UNSPECIFIED_BEHAVIOR
# warning "this is deprecated, use _LIBCPP_ASSERTIONS_ENABLE_DEBUG_RANDOMIZE_UNSPECIFIED_BEHAVIOR"
#endif
```
Or maybe we can mark the macro as deprecated with an attribute, it seems to ring a bell but I'm not sure anymore.
================
Comment at: libcxx/include/__config_hardening:84-95
+// The assertions machinery.
+
+#ifdef _LIBCPP_ENABLE_HARDENING_ASSERTIONS
+# define _LIBCPP_HARDENING_ASSERT(expression, message) \
+ (__builtin_expect(static_cast<bool>(expression), 1) \
+ ? (void)0 \
+ : _LIBCPP_VERBOSE_ABORT( \
----------------
I think we should just use `_LIBCPP_ASSERT` here. `_LIBCPP_ASSERT` isn't tied to the debug mode anymore, it's just a self-standing utility for assertions in libc++ and we should use it universally.
```
#ifdef _LIBCPP_ENABLE_HARDENING_ASSERTIONS
# define _LIBCPP_HARDENING_ASSERT(...) _LIBCPP_ASSERT(__VA_ARGS__)
#else
# define _LIBCPP_HARDENING_ASSERT(...) /* nothing */
#endif
```
================
Comment at: libcxx/include/__config_hardening:102
+# define _LIBCPP_ASSERT_CHECK_INVALID_RANGE1(range, message) \
+ _LIBCPP_HARDENING_ASSERT(__hardening::_CheckInvalidRange(range), message);
+# define _LIBCPP_ASSERT_CHECK_INVALID_RANGE2(first, last, message) \
----------------
We should force ourselves to put a semicolon after using the macro, otherwise editors will be confused.
================
Comment at: libcxx/include/__config_site.in:41
+#cmakedefine01 _LIBCPP_ENABLE_HARDENED_MODE_ABI_BREAK
+#cmakedefine01 _LIBCPP_ENABLE_HARDENED_MODE
+#cmakedefine01 _LIBCPP_ENABLE_NEW_DEBUG_MODE
----------------
Some notes and questions:
1. We don't want `_ASSERTIONS` in the names of the top-level macros, e.g. it should be `_LIBCPP_ENABLE_HARDENED_MODE` and not `_LIBCPP_ASSERTIONS_ENABLE_HARDENED_MODE`, right? What about `_LIBCPP_ASSERTIONS_ENABLE_HARDENING_ASSERTIONS`?
2. I'm not 100% happy with the names `_LIBCPP_ENABLE_HARDENED_MODE_ABI_BREAK` and `_LIBCPP_ENABLE_HARDENED_MODE`. They look obviously related, but the relationship isn't obvious.
3. For now I went with `_LIBCPP_ENABLE_NEW_DEBUG_MODE` but this definitely could be improved.
4. How do CMake definitions from this file interact with what's passed from the command line? I'm getting lots of warnings/errors about macros being undefined or redefined, but I'm probably doing the configuration wrong.
================
Comment at: libcxx/include/__config_site.in:41
+#cmakedefine01 _LIBCPP_ENABLE_HARDENED_MODE_ABI_BREAK
+#cmakedefine01 _LIBCPP_ENABLE_HARDENED_MODE
+#cmakedefine01 _LIBCPP_ENABLE_NEW_DEBUG_MODE
----------------
var-const wrote:
> Some notes and questions:
> 1. We don't want `_ASSERTIONS` in the names of the top-level macros, e.g. it should be `_LIBCPP_ENABLE_HARDENED_MODE` and not `_LIBCPP_ASSERTIONS_ENABLE_HARDENED_MODE`, right? What about `_LIBCPP_ASSERTIONS_ENABLE_HARDENING_ASSERTIONS`?
> 2. I'm not 100% happy with the names `_LIBCPP_ENABLE_HARDENED_MODE_ABI_BREAK` and `_LIBCPP_ENABLE_HARDENED_MODE`. They look obviously related, but the relationship isn't obvious.
> 3. For now I went with `_LIBCPP_ENABLE_NEW_DEBUG_MODE` but this definitely could be improved.
> 4. How do CMake definitions from this file interact with what's passed from the command line? I'm getting lots of warnings/errors about macros being undefined or redefined, but I'm probably doing the configuration wrong.
> We don't want _ASSERTIONS in the names of the top-level macros, e.g. it should be `_LIBCPP_ENABLE_HARDENED_MODE` and not `_LIBCPP_ASSERTIONS_ENABLE_HARDENED_MODE`, right?
I would agree with that.
> I'm not 100% happy with the names _LIBCPP_ENABLE_HARDENED_MODE_ABI_BREAK and _LIBCPP_ENABLE_HARDENED_MODE. They look obviously related, but the relationship isn't obvious.
As I suggested, I think we can remove `_LIBCPP_ENABLE_HARDENED_MODE_ABI_BREAK` entirely, at least for now. This makes the issue moot.
================
Comment at: libcxx/include/__config_site.in:45
+#cmakedefine01 _LIBCPP_ASSERTIONS_ENABLE_BOUNDED_ITERATORS
+#cmakedefine01 _LIBCPP_ASSERTIONS_ENABLE_CHECK_INVALID_RANGE
+#cmakedefine01 _LIBCPP_ASSERTIONS_ENABLE_CHECK_BAD_CONTAINER_ACCESS
----------------
Do we want lower-level macros like `_LIBCPP_ASSERTIONS_ENABLE_CHECK_INVALID_RANGE` here?
================
Comment at: libcxx/include/__config_site.in:45
+#cmakedefine01 _LIBCPP_ASSERTIONS_ENABLE_BOUNDED_ITERATORS
+#cmakedefine01 _LIBCPP_ASSERTIONS_ENABLE_CHECK_INVALID_RANGE
+#cmakedefine01 _LIBCPP_ASSERTIONS_ENABLE_CHECK_BAD_CONTAINER_ACCESS
----------------
var-const wrote:
> Do we want lower-level macros like `_LIBCPP_ASSERTIONS_ENABLE_CHECK_INVALID_RANGE` here?
I don't think it makes sense for those to be settable at configure time in the `__config_site.in`. I think I would only define the default value we want for the debug and hardened mode in here. Basically, vendors could decide whether they want a specific build of libc++ to enable the debug/hardened mode by default (users could still override it) by setting it in CMake. For example you might want to do:
```
#cmakedefine01 _LIBCPP_ENABLE_HARDENED_MODE_DEFAULT
#cmakedefine01 _LIBCPP_ENABLE_DEBUG_MODE_DEFAULT
```
Those would be set from CMake and would provide the default value to use for `_LIBCPP_ENABLE_HARDENED_MODE` resp `_LIBCPP_ENABLE_DEBUG_MODE` inside `__config`.
================
Comment at: libcxx/include/__config_site.in:47
+#cmakedefine01 _LIBCPP_ASSERTIONS_ENABLE_CHECK_BAD_CONTAINER_ACCESS
+#cmakedefine01 _LIBCPP_ASSERTIONS_ENABLE_FEATURE_RANDOMIZE_UNSPECIFIED_BEHAVIOR
+#cmakedefine _LIBCPP_ASSERTIONS_ENABLE_FEATURE_RANDOMIZE_UNSPECIFIED_BEHAVIOR_SEED
----------------
I used the word "feature" here since it's not directly a check, rather it enables a certain behavior. Also, this name is pretty verbose, very open to suggestions on how to shorten it.
================
Comment at: libcxx/include/__debug_utils/randomize_range.h:14
-#ifdef _LIBCPP_DEBUG_RANDOMIZE_UNSPECIFIED_STABILITY
+#if _LIBCPP_ASSERTIONS_ENABLE_FEATURE_RANDOMIZE_UNSPECIFIED_BEHAVIOR
# include <__algorithm/shuffle.h>
----------------
To reduce the size of this patch, I would suggest that we "onboard" features into the hardened/debug mode in separate patches. This patch can introduce the basic functionality, but then we can have a separate patch that simply moves `_LIBCPP_DEBUG_RANDOMIZE_UNSPECIFIED_STABILITY` from the old debug mode into the "new debug mode".
I think doing those piecemeal is going to make this patch much easier to review, but also give us space to consider important things like transitions and temporary backwards compatibility for existing macros.
================
Comment at: libcxx/include/__hardening.h:26
+_LIBCPP_HIDE_FROM_ABI
+constexpr bool __refer_to_same_range(const _Iter&, const _Iter&) {
+ return true;
----------------
Should this be part of some kind of `__hardening_traits` class? Would there be anything else in there other than `__refer_to_same_range`?
================
Comment at: libcxx/include/__hardening.h:26
+_LIBCPP_HIDE_FROM_ABI
+constexpr bool __refer_to_same_range(const _Iter&, const _Iter&) {
+ return true;
----------------
ldionne wrote:
> Should this be part of some kind of `__hardening_traits` class? Would there be anything else in there other than `__refer_to_same_range`?
Done.
My first idea was to rely on overload resolution to find the best match given that `_Iter` is a dependent name. However, that needs ADL to work which I coincidentally disabled by explicitly qualifying the function name with `std::`. I think using a traits class is more convenient regardless, thanks for the suggestion!
================
Comment at: libcxx/include/span:241
+ _LIBCPP_ASSERT_CHECK_BAD_CONTAINER_ACCESS(_Extent == __count, "span::span(Iter, Sent)");
+ _LIBCPP_ASSERT_CHECK_INVALID_RANGE(__first, __count, "span::span(Iter, size_type)");
}
----------------
I find this potentially confusing. The name `_LIBCPP_ASSERT_CHECK_INVALID_RANGE` makes it look as if we're checking that the range is invalid, when we're trying to do the reverse. Should this be called `_LIBCPP_ASSERT_CHECK_VALID_RANGE` instead (and similarly for other types of checks)?
================
Comment at: libcxx/test/std/containers/views/views.span/span.cons/iterator_sentinel.pass.cpp:117
friend bool operator==(const throw_operator_minus& x, const throw_operator_minus& y) { return x.it_ == y.it_; }
- friend bool operator<=>(const throw_operator_minus& x, const throw_operator_minus& y) { return x.it_ <=> y.it_; }
+ friend auto operator<=>(const throw_operator_minus& x, const throw_operator_minus& y) { return x.it_ <=> y.it_; }
};
----------------
Looks like this operator wasn't invoked before.
================
Comment at: libcxx/test/std/containers/views/views.span/span.cons/iterator_sentinel.pass.cpp:117
friend bool operator==(const throw_operator_minus& x, const throw_operator_minus& y) { return x.it_ == y.it_; }
- friend bool operator<=>(const throw_operator_minus& x, const throw_operator_minus& y) { return x.it_ <=> y.it_; }
+ friend auto operator<=>(const throw_operator_minus& x, const throw_operator_minus& y) { return x.it_ <=> y.it_; }
};
----------------
var-const wrote:
> Looks like this operator wasn't invoked before.
Let's land this as a NFC before this patch. Actually, is there a reason for defining it at all if it's not used?
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D151637
Files:
libcxx/docs/DesignDocs/UnspecifiedBehaviorRandomization.rst
libcxx/include/CMakeLists.txt
libcxx/include/__algorithm/shuffle.h
libcxx/include/__config
libcxx/include/__config_site.in
libcxx/include/__debug_utils/randomize_range.h
libcxx/include/__hardening
libcxx/include/__iterator/bounded_iter.h
libcxx/include/span
libcxx/include/vector
libcxx/test/libcxx/algorithms/nth_element_stability.pass.cpp
libcxx/test/libcxx/algorithms/partial_sort_stability.pass.cpp
libcxx/test/libcxx/algorithms/sort_stability.pass.cpp
libcxx/test/libcxx/assertions/hardened_assertions_disabled.pass.cpp
libcxx/test/libcxx/assertions/hardened_mode_disabled.pass.cpp
libcxx/test/std/containers/views/views.span/span.cons/iterator_sentinel.pass.cpp
-------------- next part --------------
A non-text attachment was scrubbed...
Name: D151637.533507.patch
Type: text/x-patch
Size: 44144 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/libcxx-commits/attachments/20230622/1798412d/attachment-0001.bin>
More information about the libcxx-commits
mailing list