Skip to content

Completing ChannelList Iterator API#2384

Open
Arcmantis18 wants to merge 3 commits intoAcademySoftwareFoundation:mainfrom
Arcmantis18:arcmantis/complete-channellist-iterator-api
Open

Completing ChannelList Iterator API#2384
Arcmantis18 wants to merge 3 commits intoAcademySoftwareFoundation:mainfrom
Arcmantis18:arcmantis/complete-channellist-iterator-api

Conversation

@Arcmantis18
Copy link
Copy Markdown
Contributor

@Arcmantis18 Arcmantis18 commented Apr 24, 2026

Hello

WIP on completing the Iterator API for ChannelList.

Should I create a new unit-test for the ChannelList iterators?

@Arcmantis18 Arcmantis18 marked this pull request as draft April 24, 2026 17:41
@Arcmantis18 Arcmantis18 force-pushed the arcmantis/complete-channellist-iterator-api branch from 71afd2d to cbf4b6a Compare April 24, 2026 18:16
Copy link
Copy Markdown
Member

@cary-ilm cary-ilm left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! I made some suggestions to align with std::iterator_traits

And yes, there should be unit tests in src/test/OpenEXRTest.

Comment thread src/lib/OpenEXR/ImfChannelList.h Outdated
Comment thread src/lib/OpenEXR/ImfChannelList.h
@cary-ilm
Copy link
Copy Markdown
Member

And the FreeBSD CI check failure is unrelated, probably a problem with GitHub that will hopefully go away.

@Arcmantis18 Arcmantis18 force-pushed the arcmantis/complete-channellist-iterator-api branch from cbf4b6a to 451571d Compare April 27, 2026 12:33
@cary-ilm
Copy link
Copy Markdown
Member

I don't see an obvious error, but the Windows Debug CI job is timing out, is it possible that the loop is somehow never terminating in that case? I've re-run the CI job twice now with the same results, so it appears it's more than a random glitch.

@Arcmantis18 Arcmantis18 force-pushed the arcmantis/complete-channellist-iterator-api branch 7 times, most recently from 8c46489 to b21ab7b Compare April 28, 2026 11:10
@Arcmantis18
Copy link
Copy Markdown
Contributor Author

Arcmantis18 commented Apr 28, 2026

Unfortunately I do not have a Windows machine to test myself, but I am narrowing the problem down to the '!=' global overloads...

@Arcmantis18 Arcmantis18 force-pushed the arcmantis/complete-channellist-iterator-api branch 5 times, most recently from 58751ce to 9846aff Compare April 28, 2026 14:57
@cary-ilm
Copy link
Copy Markdown
Member

One strategy for this kind of situation is to create a second branch off of the PR branch, push commits to that, and watch the logs in the CI runs in your fork. Once things work there, squash/cherry-pick the changes back into the PR branch. You'll need to enable workflow runs in your fork, since they're off by default.

It's good practice to enable workflows in your fork, then confirm the CI checks pass there before submitting a PR, to reduce noise on the main repo.

And if you're dealing with a failure in only one run, you can disable the others, or simply hack the workflow .yml files in your branch so it runs only specifically what you need, then discard those changes once things work, before you submit. If you're only interested in a single test, you can hack the ctest line in ci_steps.yml to do something likectest -R OpenEXR.testChannels, so things run much quicker.

@Arcmantis18 Arcmantis18 force-pushed the arcmantis/complete-channellist-iterator-api branch from 9846aff to d761375 Compare April 28, 2026 16:19
@Arcmantis18
Copy link
Copy Markdown
Contributor Author

Thanks for the advice, will do from now on.

…t::Iterator and ChannelList::ConstIterator. Also added global operator overloads '==' and '!=' for Iterator class.

(issue AcademySoftwareFoundation#1325)
Signed-off-by: Aries Moczar <arcmantis@protulae.com>
…mparisons. Added Iterator and ConstIterator traits.

Signed-off-by: Aries Moczar <arcmantis@protulae.com>
…elList::Iterator/ConstIterator

Signed-off-by: Aries Moczar <arcmantis@protulae.com>
@Arcmantis18 Arcmantis18 force-pushed the arcmantis/complete-channellist-iterator-api branch from d761375 to 652aa0c Compare April 29, 2026 21:27
@Arcmantis18
Copy link
Copy Markdown
Contributor Author

Arcmantis18 commented Apr 29, 2026

OK, apparently comparing iterators from different containers, even iterators from two instances of the same container class, is undefined (issue 446). Users should only be comparing iterators of the same underlying instance. I have accordingly changed my unit-test code to properly compare iterators (both Iterator and ConstIterator) from the same ChannelList.

@Arcmantis18 Arcmantis18 marked this pull request as ready for review April 29, 2026 21:44
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