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

Initial work to start switch to core use in C++ side #1719

Closed
wants to merge 12 commits into from

Conversation

kdt3rd
Copy link
Contributor

@kdt3rd kdt3rd commented Apr 16, 2024

Each commit will hopefully break down what are going to be some large changes. This represents what will be a longer chain of conversion of the C++ library to use the core library internally to gain the benefits present there.

Website preview: https://openexr--1719.org.readthedocs.build/en/1719/

Add additional API for querying information from the context for
integration into the C++ code base. These should be reviewed prior to
final release whether they need to persist or are only temporary

Signed-off-by: Kimball Thurston <[email protected]>
These are not final, but should represent concepts we will use in new
world of using the core to replace the internals of the C++

Signed-off-by: Kimball Thurston <[email protected]>
This is not complete (some of the dependencies are circular, so depends
on other later conversions to finish), so represents a WIP state

Signed-off-by: Kimball Thurston <[email protected]>
Not sure when that got added to the C++ library, but wasn't in the core

Signed-off-by: Kimball Thurston <[email protected]>
Add additional constructor to opaque attribute to enable this.
Registered custom attributes still TODO

Signed-off-by: Kimball Thurston <[email protected]>
Long name attribute flags was not being set correctly and the rules
around when there is a mismatch between the bit flags in the header and
the type attribute were not being correctly handled

Signed-off-by: Kimball Thurston <[email protected]>
Signed-off-by: Kimball Thurston <[email protected]>
@@ -742,8 +742,8 @@ exr_read_scanline_chunk_info (

if (!cinfo) return ctxt->standard_error (ctxt, EXR_ERR_INVALID_ARGUMENT);

if (part->storage_mode == EXR_STORAGE_TILED ||
Copy link
Contributor

Choose a reason for hiding this comment

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

yikes! I think this a bug fix to fastrack to main

Copy link
Contributor

@meshula meshula left a comment

Choose a reason for hiding this comment

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

I realize this is all ground work, but I see a reasonable amount of "yikes! bug fixed!" going on here. If you wanna grow this PR for a while, could I ask that we fasttrack the two "yikes" bugs I marked in core to main?

@@ -1002,8 +1002,8 @@ exr_read_tile_chunk_info (

if (!cinfo) return ctxt->standard_error (ctxt, EXR_ERR_INVALID_ARGUMENT);

if (part->storage_mode == EXR_STORAGE_SCANLINE ||
Copy link
Contributor

Choose a reason for hiding this comment

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

yikes! I think this a bug fix to fastrack to main

@@ -1676,8 +1683,13 @@ check_populate_version (

attrsz = (int32_t) one_to_native32 ((uint32_t) attrsz);
if (attrsz != 1)
return ctxt->print_error (
Copy link
Contributor

Choose a reason for hiding this comment

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

possible fastrack

@@ -144,6 +145,10 @@ InputFile::InputFile (InputPartData* part)
initialize ();
}

InputFile::~InputFile ()
Copy link
Contributor

Choose a reason for hiding this comment

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

are you gonna put something in here? this might be just an =default in the header?

Copy link
Contributor Author

@kdt3rd kdt3rd Apr 18, 2024

Choose a reason for hiding this comment

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

oh, probably, I always get weird with virtual tables and force them to be in a specific translation unit. However, as soon as I remove the generic input file base class, will remove the vtable entirely, it is not needed / helpful (and will switch to rule of 0)

@kdt3rd
Copy link
Contributor Author

kdt3rd commented Apr 18, 2024

yes, will talk around my plan at the TSC meeting tomorrow

@kdt3rd
Copy link
Contributor Author

kdt3rd commented Apr 20, 2024

splitting apart and moving to new branch, closing this PR

@kdt3rd kdt3rd closed this Apr 20, 2024
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