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

Clean up enum declarations in OpenEXRCore #1134

Merged
merged 4 commits into from
Aug 30, 2021

Conversation

cary-ilm
Copy link
Member

  • Add "exr_" prefix to transcoding_pipeline_buffer_id, and add typedef
  • Add typedef for exr_default_write_mode (exr_default_write_mode_t)
  • Add typedef for exr_attr_list_access_mode (exr_attr_list_access_mode_t)

Signed-off-by: Cary Phillips cary@ilm.com

* Add "exr_" prefix to transcoding_pipeline_buffer_id, and add typedef
* Add typedef for exr_default_write_mode (exr_default_write_mode_t)
* Add typedef for exr_attr_list_access_mode (exr_attr_list_access_mode_t)

Signed-off-by: Cary Phillips <cary@ilm.com>
Signed-off-by: Cary Phillips <cary@ilm.com>
Signed-off-by: Cary Phillips <cary@ilm.com>
@@ -28,7 +28,7 @@ enum transcoding_pipeline_buffer_id
EXR_TRANSCODE_BUFFER_SCRATCH2,
EXR_TRANSCODE_BUFFER_PACKED_SAMPLES,
EXR_TRANSCODE_BUFFER_SAMPLES
};
} exr_transcoding_pipeline_buffer_id_t;
Copy link
Contributor

Choose a reason for hiding this comment

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

Did we just break source compatibility here? We're not supposed to do that until 4.0.

To keep back compatibility, we need a typedef to make the old name equivalent to the new.

Unless we are saying that the whole OpenEXRCore library is experimental and not officially part of the public API yet, and therefore ok to change.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we discussed treating the Core library as being in a sort of WIP RFC mode? @kdt3rd does that sound right?

Copy link
Member Author

Choose a reason for hiding this comment

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

On second thought, it's easy enough to leave the enum names in place. These were inconsistently named (most enums have an associated typedef), and I thought better to straighten everything out as soon as possible, before wider use. So technically this now just adds to the API, so should be OK?

I don't think we should regard OpenEXRCore as WIP RFC, it's beyond that.

Copy link
Contributor

Choose a reason for hiding this comment

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

why would this break compatibility? We haven't changed the layout of anything, just are using the typedef instead of the enum foo, that should be fine?

Copy link
Member Author

Choose a reason for hiding this comment

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

My first commit removed the enum name and replaced it with a typedef, which would lead to a compiler error in application code already using the symbol. But this iteration should be benign since it just adds the typedef.

Signed-off-by: Cary Phillips <cary@ilm.com>
Copy link
Contributor

@kdt3rd kdt3rd 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 b62b209 into AcademySoftwareFoundation:master Aug 30, 2021
cary-ilm added a commit to cary-ilm/openexr that referenced this pull request Sep 23, 2021
…1134)

* Clean up enum declarations in OpenEXRCore

* Add "exr_" prefix to transcoding_pipeline_buffer_id, and add typedef
* Add typedef for exr_default_write_mode (exr_default_write_mode_t)
* Add typedef for exr_attr_list_access_mode (exr_attr_list_access_mode_t)

Signed-off-by: Cary Phillips <cary@ilm.com>

* cleanup

Signed-off-by: Cary Phillips <cary@ilm.com>

* cleanup #2

Signed-off-by: Cary Phillips <cary@ilm.com>

* Restore enum names, in addition to typedefs

Signed-off-by: Cary Phillips <cary@ilm.com>
cary-ilm added a commit to cary-ilm/openexr that referenced this pull request Sep 23, 2021
No ABI changes (additional enum typedefs from PR AcademySoftwareFoundation#1134, but that
shouldn't affect the ABI).

Signed-off-by: Cary Phillips <cary@ilm.com>
cary-ilm added a commit that referenced this pull request Sep 29, 2021
* Clean up enum declarations in OpenEXRCore

* Add "exr_" prefix to transcoding_pipeline_buffer_id, and add typedef
* Add typedef for exr_default_write_mode (exr_default_write_mode_t)
* Add typedef for exr_attr_list_access_mode (exr_attr_list_access_mode_t)

Signed-off-by: Cary Phillips <cary@ilm.com>

* cleanup

Signed-off-by: Cary Phillips <cary@ilm.com>

* cleanup #2

Signed-off-by: Cary Phillips <cary@ilm.com>

* Restore enum names, in addition to typedefs

Signed-off-by: Cary Phillips <cary@ilm.com>
cary-ilm added a commit that referenced this pull request Sep 29, 2021
No ABI changes (additional enum typedefs from PR #1134, but that
shouldn't affect the ABI).

Signed-off-by: Cary Phillips <cary@ilm.com>
@cary-ilm cary-ilm deleted the enum-cleanup branch November 5, 2022 22:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants