Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OpenEXR/ImfStdIO.[cpp h]: Added StdISStream. #638

Conversation

arkellr
Copy link
Contributor

@arkellr arkellr commented Jan 10, 2020

This class was missing; ie the "input" stringstream version of StdOSStream.
Based off istringstream.

Signed-off-by: Arkell Rasiah arasiah@pixsystem.com

This class was missing; ie the "input" stringstream version of StdOSStream.
Based off istringstream.

Signed-off-by: Arkell Rasiah <arasiah@pixsystem.com>
@cary-ilm cary-ilm added the Needs Discussion To be discussed in the technical steering committee label Jan 16, 2020
@cary-ilm
Copy link
Member

cary-ilm commented Feb 5, 2020

I hate to be a pest, but could you add a test for this? In IlmImfTest/testExistingStreams.cpp? It could sit alongside the existing tests.

@arkellr
Copy link
Contributor Author

arkellr commented Feb 5, 2020 via email

@cary-ilm cary-ilm added Awaiting Feedback The issue is waiting for someone to respond. and removed Needs Discussion To be discussed in the technical steering committee labels Feb 7, 2020
@cary-ilm cary-ilm added this to the Next Minor Release milestone Feb 7, 2020
@cary-ilm
Copy link
Member

Pinging @arkellr again, any chance you can find some time to put a test together for this?

@arkellr
Copy link
Contributor Author

arkellr commented Feb 26, 2020 via email

…ream.

Note I moved the "header" declaration into the scoped block that handles
the writing since thats where its only used.

Also cleaned up the compiler warning for "_pos < 0"; since _pos is an unsigned
type.

Signed-off-by: Arkell Rasiah <arasiah@pixsystem.com>
@arkellr
Copy link
Contributor Author

arkellr commented Feb 26, 2020

Cary test case added for Imf::StdOSStream/StdISStream... let me know if you would like to do a zoom to review the updates to testExistingStreams.cpp.

Copy link
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.

LGTM

@cary-ilm cary-ilm merged commit d2deb6d into AcademySoftwareFoundation:master Feb 27, 2020
@cary-ilm cary-ilm modified the milestones: Next Minor Release, v2.5.0 Apr 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Feedback The issue is waiting for someone to respond. v2.5.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants