Skip to content

Conversation

maflcko
Copy link
Member

@maflcko maflcko commented Jan 18, 2024

Currently prevector iterators have many issues:

  • Forward iterators (and stronger) must be default constructible (https://eel.is/c++draft/forward.iterators#1.2). Otherwise, some functions can not be instantiated, like std::minmax_element.
  • Various const issues with random access iterators. For example, a const iterator is different from a const_iterator, because the first one holds a mutable reference and must also return it without const. Also, operator+ must be callable regardless of the iterator object's const-ness.
  • When adding an offset to random access iterators, both x+n and n+x must be specified, see https://eel.is/c++draft/random.access.iterators#tab:randomaccessiterator

Fix all issues.

Also, upgrade the std::random_access_iterator_tag (C++17) to std::contiguous_iterator_tag (C++20)

@DrahtBot
Copy link
Contributor

DrahtBot commented Jan 18, 2024

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK TheCharlatan, stickies-v, willcl-ark

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #29119 (refactor: Use std::span over Span by maflcko)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@DrahtBot
Copy link
Contributor

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the
documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

Debug: https://github.com/bitcoin/bitcoin/runs/20620445160

@maflcko
Copy link
Member Author

maflcko commented Jan 18, 2024

Fixed libc++ CIs

@luke-jr
Copy link
Member

luke-jr commented Jan 23, 2024

Can we split the fixes from the C++20 stuff? The latter shouldn't be backported...

@maflcko
Copy link
Member Author

maflcko commented Jan 23, 2024

While it is safe to backport, I don't anything should be backported, unless there is a reason and need. A missing (default) constructor, if it was needed, would result in a compile failure. Given that the previous releases compile, this is not needed. Similar arguments can be made for the other commits.

In any case, every commit is already a separate commit, so it is not possible to split further. Anyone is free to backport any or all commits, or none at all.

Copy link
Contributor

@TheCharlatan TheCharlatan left a comment

Choose a reason for hiding this comment

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

ACK fad74bb

Could the bitdeque iterator also get std::contiguous_iterator_tag?

@maflcko
Copy link
Member Author

maflcko commented Jan 29, 2024

Could the bitdeque iterator also get std::contiguous_iterator_tag?

No. As opposed to std::vector, the elements of a deque are not stored contiguously: typical implementations use a sequence of individually allocated fixed-size arrays, with additional bookkeeping, which means indexed access to deque must perform two pointer dereferences, compared to vector's indexed access which performs only one. (Copied from source: https://en.cppreference.com/w/cpp/container/deque)

Also, even if a deque was contiguous, only bitdeque::Iterator::deque_iterator would be contiguous. Not bitdeque::Iterator, because std::addressof(std::bitset::reference) is deleted.

@fanquake fanquake requested a review from willcl-ark January 31, 2024 10:34
Copy link
Contributor

@stickies-v stickies-v left a comment

Choose a reason for hiding this comment

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

ACK fad74bb

Copy link
Member

@willcl-ark willcl-ark left a comment

Choose a reason for hiding this comment

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

ACK fad74bb

Nice enhancements to safety, usability and standards.

@fanquake fanquake merged commit f879c1b into bitcoin:master Feb 1, 2024
@maflcko maflcko deleted the 2401-prev-it- branch February 1, 2024 17:32
kwvg added a commit to kwvg/dash that referenced this pull request Nov 2, 2024
@bitcoin bitcoin locked and limited conversation to collaborators Jan 31, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants