Skip to content

Conversation

BenPope
Copy link
Member

@BenPope BenPope commented Oct 9, 2025

Some improvements to chunked_vector:

  • Satisfy random_access_iterator concept

Backports Required

  • none - not a bug fix
  • none - this is a backport
  • none - issue does not exist in previous branches
  • none - papercut/not impactful enough to backport
  • v25.2.x
  • v25.1.x
  • v24.3.x

Release Notes

  • none

@Copilot Copilot AI review requested due to automatic review settings October 9, 2025 12:36
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR improves the chunked_vector iterator implementation to properly satisfy the C++20 random access iterator concept requirements. The changes ensure the iterator type aliases are correctly defined to support both const and non-const iterators while handling the special case of bool type.

Key Changes

  • Added iterator_concept type alias to explicitly declare random access iterator support
  • Fixed value_type to always be T (not conditionally const)
  • Corrected pointer and reference type aliases to properly handle const/non-const variants

@BenPope BenPope changed the title chunked_vector: Satisfy random_access_iterator concept chunked_vector: Various improvements Oct 9, 2025
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we static assert this in tests?

Copy link
Member Author

Choose a reason for hiding this comment

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

Can we static assert this in tests?

Yeah, I think there's some more to come, but I'm not full time on it.

Copy link
Member

@dotnwat dotnwat left a comment

Choose a reason for hiding this comment

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

i don't think this will work, right? doesn't random access iterators imply certain properties about the memory layout? stuff like for iterators a and b you can do pointer arithemetic with them that acts like the memory is contiguous?

@rockwotj
Copy link
Contributor

rockwotj commented Oct 9, 2025

i don't think this will work, right? doesn't random access iterators imply certain properties about the memory layout? stuff like for iterators a and b you can do pointer arithemetic with them that acts like the memory is contiguous?

There should be https://en.cppreference.com/w/cpp/iterator/contiguous_iterator.html for the difference between random access and contiguous, but maybe not everything respects that properly

@dotnwat
Copy link
Member

dotnwat commented Oct 13, 2025

i don't think this will work, right? doesn't random access iterators imply certain properties about the memory layout? stuff like for iterators a and b you can do pointer arithemetic with them that acts like the memory is contiguous?

There should be https://en.cppreference.com/w/cpp/iterator/contiguous_iterator.html for the difference between random access and contiguous, but maybe not everything respects that properly

Not sure, but I the second bullet point under semantic requirements here https://en.cppreference.com/w/cpp/iterator/random_access_iterator.html is where I saw the pointer difference idea.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants