Skip to content

Conversation

@ednolan
Copy link
Member

@ednolan ednolan commented Nov 14, 2024

The wording of P2727R4 states, with respect to the iterator_category type alias:

"The nested type iterator_category is defined if and only if IteratorConcept is derived from forward_iterator_tag."

Previously, this implementation had attempted to implement this using a class template detail::iter_cat, which had a specialization that contained an iterator_category member type alias which was only enabled if IteratorConcept was derived from
std::forward_iterator_tag. However, the iter_category member type alias of iterator_interface itself was specified as detail::iter_cat</* ... */>>::iterator_category, which caused iterator_interface to simply fail to compile if IteratorConcept was not derived from forward_iterator_tag as detail::iter_cat's iterator_category was not found.

This commit addresses the issue by removing the iterator_category member type alias from iterator_interface itself and making iterator_interface provide the member type alias by inheriting from detail::iter_cat, which satisfies the requirement in the wording. It also adds a reproducing unit test.

Copy link
Member

@camio camio left a comment

Choose a reason for hiding this comment

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

Approved with one minor suggestion for the unit test.

@camio
Copy link
Member

camio commented Nov 14, 2024

Should this behavior also apply when using ext_iterator_interface_compat? If so, maybe the unit test could use that instead to ensure we have all bases covered (and we can eliminate the #if).

The wording of P2727R4 states, with respect to the iterator_category
type alias:

"The nested type iterator_category is defined if and only if
IteratorConcept is derived from forward_iterator_tag."

Previously, this implementation had attempted to implement this using
a class template detail::iter_cat, which had a specialization that
contained an iterator_category member type alias which was only
enabled if IteratorConcept was derived from
std::forward_iterator_tag. However, the iter_category member type
alias of iterator_interface itself was specified as
detail::iter_cat</* ... */>>::iterator_category, which caused
iterator_interface to simply fail to compile if IteratorConcept was
not derived from forward_iterator_tag as detail::iter_cat's
iterator_category was not found.

This commit addresses the issue by removing the iterator_category
member type alias from iterator_interface itself and making
iterator_interface provide the member type alias by inheriting from
detail::iter_cat, which satisfies the requirement in the wording. It
also adds a reproducing unit test.
@ednolan
Copy link
Member Author

ednolan commented Nov 14, 2024

Should this behavior also apply when using ext_iterator_interface_compat? If so, maybe the unit test could use that instead to ensure we have all bases covered (and we can eliminate the #if).

Done

@ednolan ednolan merged commit 47c8faa into bemanproject:main Nov 14, 2024
7 checks passed
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