Skip to content

Conversation

mgovers
Copy link
Member

@mgovers mgovers commented Oct 3, 2025

This makes sure that iterator facade no longer uses CRTP but instead the more modern "deducing this" pattern, which does not require complicated two-way-friend derivation patterns and templated inheritance. Constraints on the facadeable type are enforced by the (now public) explicit constructor.

Signed-off-by: Martijn Govers <[email protected]>
Signed-off-by: Martijn Govers <[email protected]>
Signed-off-by: Martijn Govers <[email protected]>
Signed-off-by: Martijn Govers <[email protected]>
Signed-off-by: Martijn Govers <[email protected]>
@mgovers mgovers self-assigned this Oct 3, 2025
@mgovers mgovers added the improvement Improvement on internal implementation label Oct 3, 2025
Signed-off-by: Martijn Govers <[email protected]>
Signed-off-by: Martijn Govers <[email protected]>
Copy link
Member

@figueroa1395 figueroa1395 left a comment

Choose a reason for hiding this comment

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

The review might have been done on some outdated parts, although I don't expect the latter commits to be content-significant (rather small compiling fixes). That said, very good job, deducing this is really a nice thing. Besides the minor comments, LGTM.

Signed-off-by: Martijn Govers <[email protected]>
Signed-off-by: Martijn Govers <[email protected]>
Signed-off-by: Martijn Govers <[email protected]>
…ifference operators in derived iterator implementation

Signed-off-by: Martijn Govers <[email protected]>
Signed-off-by: Martijn Govers <[email protected]>
Signed-off-by: Martijn Govers <[email protected]>
Signed-off-by: Martijn Govers <[email protected]>
Copy link

sonarqubecloud bot commented Oct 9, 2025

@mgovers
Copy link
Member Author

mgovers commented Oct 9, 2025

sonar cloud issues are false-positives

Copy link
Member

@figueroa1395 figueroa1395 left a comment

Choose a reason for hiding this comment

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

Approving but adding do not merge label because Friday. Feel free to merge on Monday.

@figueroa1395 figueroa1395 added the do-not-merge This should not be merged label Oct 10, 2025
@TonyXiang8787 TonyXiang8787 removed the do-not-merge This should not be merged label Oct 13, 2025
@TonyXiang8787 TonyXiang8787 added this pull request to the merge queue Oct 13, 2025
Merged via the queue into main with commit 8e8fd63 Oct 13, 2025
32 of 33 checks passed
@TonyXiang8787 TonyXiang8787 deleted the feature/iterator-facade-deducing-this branch October 13, 2025 07:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

improvement Improvement on internal implementation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants