Skip to content

refactor: implement c++23 inspired ToUnderlying#5210

Merged
UdjinM6 merged 6 commits intodashpay:developfrom
PastaPastaPasta:refac/tounderlying
Feb 20, 2023
Merged

refactor: implement c++23 inspired ToUnderlying#5210
UdjinM6 merged 6 commits intodashpay:developfrom
PastaPastaPasta:refac/tounderlying

Conversation

@PastaPastaPasta
Copy link
Member

Issue being fixed or feature implemented

Avoid lots of static_cast's from enums to underlying types. Communicate intention better

What was done?

implement c++23 inspired ToUnderlying, then see std::to_underlying and https://en.cppreference.com/w/cpp/types/underlying_type; Then, we use this instead of static_casts for enums -> underlying type

How Has This Been Tested?

make check

Breaking Changes

None

Checklist:

  • 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

For repository code-owners and collaborators only

  • I have assigned this pull request to a milestone

@PastaPastaPasta PastaPastaPasta added this to the 19 milestone Feb 17, 2023
see std::to_underlying and https://en.cppreference.com/w/cpp/types/underlying_type
Then, we use this instead of static_casts for enums -> underlying type
@PastaPastaPasta
Copy link
Member Author

I'm really confused why this build fails... Please help 🙈

@UdjinM6
Copy link

UdjinM6 commented Feb 18, 2023

I'm really confused why this build fails... Please help 🙈

git add src/util/underlying.h ;)

@PastaPastaPasta
Copy link
Member Author

I'm really confused why this build fails... Please help 🙈

git add src/util/underlying.h ;)

Oh 🙈 I'll do that tonight 😂

@knst
Copy link
Collaborator

knst commented Feb 19, 2023

LGTM, but let's add these also and may be this

@UdjinM6
Copy link

UdjinM6 commented Feb 19, 2023

linter complains

A new circular dependency in the form of "llmq/debug -> llmq/dkgsessionhandler -> llmq/debug" appears to have been introduced.
A new circular dependency in the form of "llmq/debug -> llmq/dkgsessionhandler -> llmq/dkgsession -> llmq/debug" appears to have been introduced.

@PastaPastaPasta
Copy link
Member Author

linter complains

A new circular dependency in the form of "llmq/debug -> llmq/dkgsessionhandler -> llmq/debug" appears to have been introduced.
A new circular dependency in the form of "llmq/debug -> llmq/dkgsessionhandler -> llmq/dkgsession -> llmq/debug" appears to have been introduced.

resolved

@knst
Copy link
Collaborator

knst commented Feb 20, 2023

utACK

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.

LGTM, utACK

@UdjinM6 UdjinM6 merged commit 0ee3974 into dashpay:develop Feb 20, 2023
@PastaPastaPasta PastaPastaPasta deleted the refac/tounderlying branch February 20, 2023 15:19
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