Skip to content

refactor: implement irange::range and use it#4788

Merged
UdjinM6 merged 6 commits intodashpay:developfrom
PastaPastaPasta:refac-irange
Apr 25, 2022
Merged

refactor: implement irange::range and use it#4788
UdjinM6 merged 6 commits intodashpay:developfrom
PastaPastaPasta:refac-irange

Conversation

@PastaPastaPasta
Copy link
Member

No description provided.

@PastaPastaPasta PastaPastaPasta requested a review from UdjinM6 April 20, 2022 18:30
Copy link
Collaborator

Choose a reason for hiding this comment

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

along with referring original implementation as a source, adding their copyright is probably for the best, also mention the commit of cpp_range from which this was derived from

Copy link
Member Author

Choose a reason for hiding this comment

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

They didn't have a copyright on the file, just a MIT license in the folder

@PastaPastaPasta
Copy link
Member Author

see latest

@github-actions
Copy link

This pull request has conflicts, please rebase.

Copy link

Choose a reason for hiding this comment

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

Suggested change
if (!quorumThreadInterrupt) {
if (quorumThreadInterrupt) {

@PastaPastaPasta PastaPastaPasta requested a review from UdjinM6 April 20, 2022 19:24
@UdjinM6 UdjinM6 added this to the 19 milestone Apr 20, 2022
@UdjinM6
Copy link

UdjinM6 commented Apr 20, 2022

pls see 49f76c6650

@github-actions
Copy link

This pull request has conflicts, please rebase.

UdjinM6
UdjinM6 previously approved these changes Apr 25, 2022
Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

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

utACK

@UdjinM6 UdjinM6 dismissed their stale review April 25, 2022 20:38

hmm.. linter isn't happy

Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

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

utACK

@UdjinM6 UdjinM6 merged commit 80d9738 into dashpay:develop Apr 25, 2022
@PastaPastaPasta PastaPastaPasta deleted the refac-irange branch April 26, 2022 03:02
PastaPastaPasta pushed a commit that referenced this pull request Nov 27, 2023
#5732)

## Issue being fixed or feature implemented
#4788 (comment)

noticed while working on #5731

## What was done?

## How Has This Been Tested?
run a node, check logs - there is a meaningful time span between `start`
and `done` now and not just zeros all the time.

## Breaking Changes

## Checklist:
- [x] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e
tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone _(for repository
code-owners and collaborators only)_
PastaPastaPasta added a commit that referenced this pull request Mar 12, 2026
…polyfills (`util/std23.h`) and helpers (`util/helpers.h`)

1c8260c refactor: add `util::to_string(bool)` for convenience (Kittywhiskers Van Gogh)
dec2766 refactor: merge `util/ranges.h` and `util/pointer.h` to `util/helpers.h` (Kittywhiskers Van Gogh)
f57edb5 refactor: use `std23::ranges::fold_left` in Dash-specific code (Kittywhiskers Van Gogh)
4c30be5 util: add polyfill for `std::ranges::fold_left` (Kittywhiskers Van Gogh)
5dc17fc refactor: use `std23::ranges::contains` in Dash-specific code (Kittywhiskers Van Gogh)
43e64ed util: add polyfill for `std::ranges::contains` (Kittywhiskers Van Gogh)
79f0711 refactor: replace `irange::range` with thin `std::views::iota` wrapper (Kittywhiskers Van Gogh)
58a9a3d refactor: switch from `irange(min, max)` to `std::views::iota(min, max)` (Kittywhiskers Van Gogh)
6bee408 refactor: distinguish between custom helpers and polyfills (Kittywhiskers Van Gogh)
f1d5d30 refactor: move `enumerate` to C++23 polyfill, mirror namespace structure (Kittywhiskers Van Gogh)
aa12106 refactor: rename `ToUnderlying()` to match C++23 convention (Kittywhiskers Van Gogh)
2ca0fd7 fix: enum bound check in GetSimulatedErrorRate (Kittywhiskers Van Gogh)

Pull request description:

  ## Motivation

  The upgrade to C++20 ([dash#6380](#6380)) allowed us to drop most of our polyfills meant as bridge from C++17, though we still have utils beyond the ones inherited from upstream that have accumulated over time, most of them, seeking to bridge C++23 features while others are entirely custom or serve as convenience functions or thin wrappers.

  Some of them include:
  * [dash#4622](#4622) for `util/ranges.h` (mostly now an alias to [`std::ranges`](https://en.cppreference.com/w/cpp/ranges.html) after C++20 migration except for `find_if_opt`, which is custom)
  * [dash#4788](#4788) for `util/irange.h` (equivalent to [`std::views::iota`](https://en.cppreference.com/w/cpp/ranges/iota_view.html), introduced in C++20)
  * [dash#5059](#5059) for `util/enumerate.h`  (equivalent to [`std::views::enumerate`](http://en.cppreference.com/w/cpp/ranges/enumerate_view.html), introduced in C++23)
  * [dash#5210](#5210) for `util/underlying.h` (equivalent to [`std::to_underlying`](https://en.cppreference.com/w/cpp/utility/to_underlying.html), introduced in C++23)
  * 0a4e726 for `util/pointer.h` (custom `std::shared_ptr` helper)

  This pull requests compacts all of those helpers into two files:
  * `util/helpers.h` (custom helpers, replacing `util/irange.h`, `util/pointer.h`, `util/ranges.h`)
  * `util/std23.h` (polyfill for C++23, replacing `util/enumerate.h` and `util/underlying.h`)

  And additionally, introduces additional capabilities utilised in Dash-specific code:

  * `std23::ranges::contains` (equivalent to [`std::ranges::contains`](https://en.cppreference.com/w/cpp/algorithm/ranges/contains.html))
  * `std23::ranges::fold_left` (equivalent to [`std::ranges::fold_left`](https://en.cppreference.com/w/cpp/algorithm/ranges/fold_left.html))
  * `util::to_string(bool)` (equivalent to `value ? "true" : "false"`)

  ## Additional Information

  * `ToUnderlying()` needed to be renamed to `to_underlying()` to match with the standard library naming so we can discontinue the polyfill easily when we migrate to C++23 by just going `s/std23/std/g`.

  * While `iranges::range` is equivalent to `std::views::iota`, the latter requires both min and max to be of the same type but literal `0` has a type incompatible with `size_t`, requiring `size_t{0}` to compile. As propagation of these changes for each instance is bothersome, `util::irange()` exists as a thin wrapper that wraps the literal around the type for `max`

  ## Breaking Changes

  None expected.

  ## Checklist

  - [x] I have performed a self-review of my own code
  - [x] I have commented my code, particularly in hard-to-understand areas
  - [x] I have added or updated relevant unit/integration/functional/e2e tests
  - [x] I have made corresponding changes to the documentation **(note: N/A)**
  - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

ACKs for top commit:
  PastaPastaPasta:
    utACK 1c8260c; seems reasonable
  UdjinM6:
    utACK 1c8260c

Tree-SHA512: 82f4b74f304d8680ddb03a923843905a470b374c034284dbe6a013d19e67a769743965f710c5267751742488961907d346ce721d01029cffa543178fd560dcc1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants