Skip to content

Make the batcher and partition iterators usable - #103

Open
susilehtola wants to merge 1 commit into
wavefunction91:masterfrom
susilehtola:fix/batcher-const-iteration
Open

Make the batcher and partition iterators usable#103
susilehtola wants to merge 1 commit into
wavefunction91:masterfrom
susilehtola:fix/batcher-const-iteration

Conversation

@susilehtola

Copy link
Copy Markdown
Collaborator

SphericalMicroBatcher::const_iterator declared its comparison operators
taking the non-const iterator type:

bool operator==( iterator other ){ ... }

There is no conversion between the two, so comparing two const_iterators
was a hard error and the loop the class exists to support could not be
written over a const batcher. A const batcher also had no begin()/end()
at all, only cbegin()/cend(), so range-for over one failed by a second
independent route.

RadialGridPartition::rgp_iterator had the same class of bug in its
post-increment, which declares iterator retval = *this; -- the enclosing
class's non-const typedef, so const_iterator::operator++(int) could never
instantiate.

Fix both, and give the batcher const begin()/end().

While here, three related defects in the same iterators:

  • operator+ mutated *this and returned a reference to it. That is
    operator+= under the wrong name; at() worked only because it discards
    the iterator afterwards. Return a copy and take a difference_type.
  • difference_type was spelled different_type and iterator_category was
    spelled iterator_catagory, so std::iterator_traits saw neither and no
    standard algorithm could use these iterators.
  • With the traits now visible, reference = value_type& would have been
    a trap: operator* returns a prvalue, so binding a reference to it
    dangles. Use the proxy-iterator spelling instead (reference =
    value_type, pointer = void).

range() and operator* are now const, as they never mutated anything.

The existing test called cbatcher.at(i) but never iterated a const
batcher; add both a range-for and an explicit cbegin()/cend() loop.

Co-Authored-By: Claude Opus 5 (1M context) noreply@anthropic.com
Claude-Session: https://claude.ai/code/session_01FDTFYJMQ76iujDFNHzZyXF

SphericalMicroBatcher::const_iterator declared its comparison operators
taking the non-const `iterator` type:

    bool operator==( iterator other ){ ... }

There is no conversion between the two, so comparing two const_iterators
was a hard error and the loop the class exists to support could not be
written over a const batcher. A const batcher also had no begin()/end()
at all, only cbegin()/cend(), so range-for over one failed by a second
independent route.

RadialGridPartition::rgp_iterator had the same class of bug in its
post-increment, which declares `iterator retval = *this;` -- the enclosing
class's non-const typedef, so const_iterator::operator++(int) could never
instantiate.

Fix both, and give the batcher const begin()/end().

While here, three related defects in the same iterators:

* operator+ mutated *this and returned a reference to it. That is
  operator+= under the wrong name; at() worked only because it discards
  the iterator afterwards. Return a copy and take a difference_type.
* difference_type was spelled `different_type` and iterator_category was
  spelled `iterator_catagory`, so std::iterator_traits saw neither and no
  standard algorithm could use these iterators.
* With the traits now visible, `reference = value_type&` would have been
  a trap: operator* returns a prvalue, so binding a reference to it
  dangles. Use the proxy-iterator spelling instead (reference =
  value_type, pointer = void).

range() and operator* are now const, as they never mutated anything.

The existing test called cbatcher.at(i) but never iterated a const
batcher; add both a range-for and an explicit cbegin()/cend() loop.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FDTFYJMQ76iujDFNHzZyXF
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.

1 participant