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.

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

util: add ranges.h to emulate c++20 std::ranges #4622

Merged
merged 9 commits into from
Dec 21, 2021

Conversation

PastaPastaPasta
Copy link
Member

No description provided.

ci/Dockerfile.builder Outdated Show resolved Hide resolved
test/lint/lint-cppcheck-dash.sh Outdated Show resolved Hide resolved
@UdjinM6
Copy link

UdjinM6 commented Dec 19, 2021

LGTM but let's maybe split this in 2 PRs if possible:

  1. cppcheck pinning and cppcheck related fixes (move these to refactor: Fix warnings from cppcheck #4625?)
  2. ranges and stuff

@PastaPastaPasta PastaPastaPasta marked this pull request as ready for review December 20, 2021 16:24
@PastaPastaPasta PastaPastaPasta changed the title Ranges and more util: add ranges.h to emulate c++20 std::ranges Dec 20, 2021
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 added this to the 18 milestone Dec 21, 2021
@UdjinM6 UdjinM6 merged commit f2c603b into dashpay:develop Dec 21, 2021
@PastaPastaPasta PastaPastaPasta deleted the ranges-and-more branch December 21, 2021 16:16
gades pushed a commit to cosanta/cosanta-core that referenced this pull request Nov 7, 2023
* refactor: introduce ranges.h for prettier std algo. Also use it in dash core

* fix formatting, use ranges instead of std

* remove commented out code

* introduce ranges find_if_opt, count_if, find_if. Use them all, and more

* use std::accumulate

* capture everything so that threadsaftey analysis is happy

* fix linter

* fix linter

* remove pessimizing move
gades pushed a commit to piratecash/pirate that referenced this pull request Dec 9, 2023
* refactor: introduce ranges.h for prettier std algo. Also use it in dash core

* fix formatting, use ranges instead of std

* remove commented out code

* introduce ranges find_if_opt, count_if, find_if. Use them all, and more

* use std::accumulate

* capture everything so that threadsaftey analysis is happy

* fix linter

* fix linter

* remove pessimizing move
PastaPastaPasta added a commit that referenced this pull request Nov 4, 2024
…ammy`)

1592a0f ci: update containers and CI to use Ubuntu 22.04 LTS (`jammy`) (Kittywhiskers Van Gogh)
decbd05 ci: stop running `check-symbols` during builds (Kittywhiskers Van Gogh)

Pull request description:

  ## Motivation

  While some aspects of C++20 are supported by GCC 9.3 (the version shipped with `focal`, [source](https://packages.ubuntu.com/focal/g++)), P0896R4 ("The One Ranges Proposal") is not ([source](https://en.cppreference.com/w/cpp/compiler_support)), required for moving away from [dash#4622](#4622) to adopt `std::ranges` and for backporting PRs like [bitcoin#28687](bitcoin#28687).

  They're supported from GCC 10.1 onwards ([source](https://stackoverflow.com/questions/56118941/do-we-have-c20-ranges-library-in-gcc-9/56118997#56118997)) and the next available LTS, `jammy`, currently ships with GCC 11.2 ([source](https://packages.ubuntu.com/jammy/g++)). As we're now using Guix to build our releases, there shouldn't be any adverse effects from our containers having a higher version of `glibc`.

  Additionally, upstream uses 24.04 (`noble`) for their build images ([source](https://github.com/bitcoin/bitcoin/blob/a2317e27b7fb86df4e32cd1674c06e09cb808248/ci/test/00_setup_env_native_tsan.sh#L10)) and `jammy` to determine the minimum version to CMake ([source](https://github.com/bitcoin/bitcoin/blob/a2317e27b7fb86df4e32cd1674c06e09cb808248/CMakeLists.txt#L5-L6)) (for reference, `focal` ships with CMake 3.16, [source](https://packages.ubuntu.com/focal/cmake)).

  ## Additional Information

  * Builds will no longer run `check-symbols` (and the option to do so, `RUN_SYMBOL_TESTS` has been removed) as portable builds (builds expected for distributions) are built with Guix, which pins its version of `glibc` ([source](https://github.com/dashpay/dash/blob/396573d09cff5c65b2e3b4ba965185ac532b8b5c/contrib/guix/manifest.scm#L152), currently 2.28), runs `check-symbols` ([source](https://github.com/dashpay/dash/blob/396573d09cff5c65b2e3b4ba965185ac532b8b5c/contrib/guix/libexec/build.sh#L304-L305)), which in turn make sure that it doesn't contain symbols expected in versions higher than `glibc` 2.31 ([source](https://github.com/dashpay/dash/blob/396573d09cff5c65b2e3b4ba965185ac532b8b5c/contrib/devtools/symbol-check.py#L38-L42)).

    Upstream does not use `check-symbols` during their builds nor do we have a reason to (as we no longer using Gitian).

  * CI build failures w.r.t. `pthread_yield` ([build](https://github.com/dashpay/dash/actions/runs/11645989968/job/32437272177?pr=6379#step:7:3136), [build](https://github.com/dashpay/dash/actions/runs/11645989968/job/32430165640#step:7:3157)) should be resolved by rebuilding `bdb4` (see [bitcoin#26423](bitcoin#26423 (comment))). This problem has only been observed on GitHub CI and locally (the latter resolved by rebuilding depends, I use a separate target triple for testing, `x86_64-jammy-linux-gnu`), GitLab CI has not observed such failures ([build](https://gitlab.com/dashpay/dash/-/jobs/8253943710)).

  ## 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 **(note: N/A)**
  - [x] I have added or updated relevant unit/integration/functional/e2e tests **(note: N/A)**
  - [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:
  UdjinM6:
    utACK 1592a0f
  knst:
    utACK 1592a0f
  PastaPastaPasta:
    utACK 1592a0f

Tree-SHA512: cbe6505211246c711dc0fd8645b8a4c6123b5ac163341dca4c686f8905a630c57d483a91a6e29bd9e23bac79d774e339181d2cb17bb3affeb5902e5f548ffa54
PastaPastaPasta added a commit that referenced this pull request Nov 4, 2024
…as c0b716f2

f18e839 build: drop symlinks in immer subtree (Kittywhiskers Van Gogh)
d761111 build: fix gitian builds (Kittywhiskers Van Gogh)
a9f46b3 Squashed 'src/immer/' changes from 9cb6a5a845..5875f7739a (Kittywhiskers Van Gogh)
e4ee302 revert: fix gitian builds (Kittywhiskers Van Gogh)
fb00300 revert: drop symlinks in immer subtree (Kittywhiskers Van Gogh)

Pull request description:

  ## Additional Information

  * Dependency for #6380

  #6380 will be using `std::ranges` as a replacement for [dash#4622](#4622) and currently, it will fail to compile due to the current version of `immer` (last updated two years ago in #4911) not meeting constraints ([source](https://en.cppreference.com/w/cpp/language/constraints)) set by `std::ranges` (see below).

  ```
  ./evo/deterministicmns.h:243:16: error: no matching function for call to object of type 'const __count_if_fn'
          return ranges::count_if(mnMap, [](const auto& p) { return IsMNValid(*p.second); });
                 ^~~~~~~~~~~~~~~~
  /usr/bin/../lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/ranges_algo.h:331:7: note: candidate template ignored: substitution failure [with _Range = const MnMap &, _Proj = identity, _Pred = (lambda at ./evo/deterministicmns.h:243:40)]: constraints not satisfied for alias template 'range_difference_t' [with _Range = const immer::map<uint256, std::shared_ptr<const CDeterministicMN>, CDeterministicMNList::ImmerHasher> &]
        operator()(_Range&& __r, _Pred __pred, _Proj __proj = {}) const
        ^
  /usr/bin/../lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/ranges_algo.h:316:7: note: candidate function template not viable: requires at least 3 arguments, but 2 were provided
        operator()(_Iter __first, _Sent __last,
        ^
  ```

  This has been resolved by updating `immer` to the latest available release, [v0.8.1](https://github.com/arximboldi/immer/releases/tag/v0.8.1).

  Expected subtree hash **without changes** are `a5ded361aec714bc74149909c5e1984c10ab132c4c539c63fe57362dccd95556` (see [here](#6323 (review)) for verification instructions).

  ## Breaking Changes

  None expected.

  ## Checklist

  - [x] I have performed a self-review of my own code **(note: N/A)**
  - [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)**
  - [x] I have added or updated relevant unit/integration/functional/e2e tests **(note: N/A)**
  - [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 f18e839
  UdjinM6:
    utACK f18e839

Tree-SHA512: 99d1b577eb4cf3fcc3ebfd28f19006700f085d23ccdabe802a2aa5844e4b39314347b0c0e49a352c5fc034d6584a0109edf08fa3c0846514967f1fb16e7f127a
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.

2 participants