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.

Sign up for GitHub

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

Enhance _STL_VERIFY for std::barrier #3757

Merged
merged 3 commits into from
Jun 15, 2023
Merged

Enhance _STL_VERIFY for std::barrier #3757

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e5e15a7
Update 1
achabense Jun 9, 2023
02c2052
Update 2
achabense Jun 9, 2023
f10d525
Code review feedback.
StephanTLavavej Jun 10, 2023
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
7 changes: 3 additions & 4 deletions stl/inc/barrier
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ inline constexpr ptrdiff_t _Barrier_value_mask = ~_Barrier_arrival_token
inline constexpr ptrdiff_t _Barrier_value_shift = 1;
inline constexpr ptrdiff_t _Barrier_invalid_token = 0;
inline constexpr ptrdiff_t _Barrier_value_step = 1 << _Barrier_value_shift;
inline constexpr ptrdiff_t _Barrier_max = (1ULL << (sizeof(ptrdiff_t) * CHAR_BIT - 2)) - 1;
inline constexpr ptrdiff_t _Barrier_max = PTRDIFF_MAX >> _Barrier_value_shift;

template <class _Completion_function>
class _Arrival_token {
Expand Down Expand Up @@ -81,7 +81,7 @@ public:
constexpr explicit barrier(
const ptrdiff_t _Expected, _Completion_function _Fn = _Completion_function()) noexcept /* strengthened */
: _Val(_One_then_variadic_args_t{}, _STD move(_Fn), _Expected << _Barrier_value_shift) {
_STL_VERIFY(_Val._Myval2._Current.load(memory_order_relaxed) >= 0,
_STL_VERIFY(_Expected >= 0 && _Expected <= (max) (),
"Precondition: expected >= 0 and expected <= max() (N4950 [thread.barrier.class]/9)");
}

Expand All @@ -93,9 +93,8 @@ public:
}

_NODISCARD_BARRIER_TOKEN arrival_token arrive(ptrdiff_t _Update = 1) noexcept /* strengthened */ {
// Shifting before precondition check, so that exceeding max() will trigger precondition check too
_STL_VERIFY(_Update > 0 && _Update <= (max) (), "Precondition: update > 0 (N4950 [thread.barrier.class]/12)");
Copy link
Contributor Author

@achabense achabense Jun 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Sorry for this late review -- I didn't realize it was "Pending"-ed.

_Update <= max() is necessary, as it avoids large positive value becoming negative after shift. However, this will cause a mismatch between the check and error message. This check is more relevant to the constraint "update <= expected count", but I don't think we should put it off. I have no idea how to make an improvement. Any ideas?

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 okay with a slightly confusing message here.

_Update <<= _Barrier_value_shift;
_STL_VERIFY(_Update > 0, "Precondition: update > 0 (N4950 [thread.barrier.class]/12)");
// TRANSITION, GH-1133: should be memory_order_release
ptrdiff_t _Current = _Val._Myval2._Current.fetch_sub(_Update) - _Update;
_STL_VERIFY(_Current >= 0, "Precondition: update is less than or equal to the expected count "
Expand Down