-
Notifications
You must be signed in to change notification settings - Fork 659
Add OPENEXR_CORE_USE_NAMESPACE to build OpenEXRCore with C++ #2205
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
base: main
Are you sure you want to change the base?
Conversation
When `OPENEXR_CORE_USE_NAMESPACE=ON`, the .c and .h files in
OpenEXRCore are built with C++ rather than C, with all symbols in the
`OPENEXR_IMF_INTERNAL_NAMESPACE` namespace just like the OpenEXR
library.
This makes is possible to trade the benefits of the C implementation
for the benefits of C++ and the namespacing of all symbols.
All code in the OpenEXRCore .c and .h files is now bracketed inside
`OPENEXR_CORE_NAMESPACE_ENTER` and `OPENEXR_CORE_NAMESPACE_EXIT`,
which add namespace directives (e.g. `namepace Imf_3_4 {`). These
degenerate to empty when compiling with C, which is still the default.
There are also macros
`OPENEXR_CORE_EXTERN_C_ENTER/OPENEXR_CORE_EXTERN_C_EXIT` for the
OpenEXRCore code that needs `extern "C" {`, which reverts the the C++
OpenEXR namespace in C++.
This also adds missing C++ casts to OpenEXRCore, required by C++ even
though the are not necessary in C.
Also, the `_priv_exr_part_t` struct now has a custom constructor and
assignment operator because its `chunk_table` field is a
`atomic_uintptr_t`, which has those methods deleted.
This also includes some necessary changes to the vendored `libdeflate`
code to compile in C++.
When building OpenEXR with the default configuration, this produces
the same object code as before. The namespaced symbols are only
generated when configured to do so. As as test, I ran `nm` on the
shared library and got the same symbols as before.
Signed-off-by: Cary Phillips <[email protected]>
Signed-off-by: Cary Phillips <[email protected]>
|
By the way, I'm not crazy about the name of this build option, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR introduces the OPENEXR_CORE_USE_NAMESPACE build option, which allows OpenEXRCore to be compiled with C++ (instead of the default C) with all symbols placed in the same namespace as the OpenEXR library. This enables linking multiple versions of OpenEXR in the same program without symbol conflicts. The implementation adds namespace macros throughout the OpenEXRCore codebase and includes necessary C++ casts required for compilation.
Key changes:
- New CMake option
OPENEXR_CORE_USE_NAMESPACEthat switches OpenEXRCore from C to C++ - Namespace macros (
OPENEXR_CORE_NAMESPACE_ENTER/EXITandOPENEXR_CORE_EXTERN_C_ENTER/EXIT) added throughout OpenEXRCore - C++ explicit casts added where C's implicit conversions are not allowed
Reviewed changes
Copilot reviewed 99 out of 101 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| website/install.rst | Documents the new OPENEXR_CORE_USE_NAMESPACE option and linking multiple OpenEXR versions |
| cmake/OpenEXRConfig.h.in | Defines the namespace macros that conditionally add C++ namespaces |
| src/lib/OpenEXRCore/CMakeLists.txt | Adds logic to compile .c files as C++ when OPENEXR_CORE_USE_NAMESPACE is enabled |
| src/lib/OpenEXRCore/*.c | Adds namespace macros and C++ casts to all OpenEXRCore implementation files |
| src/lib/OpenEXRCore/*.h | Replaces extern "C" blocks with namespace macros in header files |
| src/test/OpenEXRCoreTest/*.cpp | Updates test files to use OPENEXR_IMF_NAMESPACE consistently |
| external/deflate/lib/*.c | Adds C++ casts to vendored libdeflate code for C++ compatibility |
| .github/workflows/*.yml | Adds CI workflow configuration to test the new option |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| static void | ||
| initializeFuncs (void) | ||
| { | ||
| static int done = 0; |
Copilot
AI
Nov 25, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The variable f16c, avx, and sse2 are declared after the early return when done is true at line 2328, but the declarations were moved to after the preprocessor conditional at line 2337. This creates a situation where the code structure is inconsistent. Consider moving the entire initialization block including the done variable inside the #else block if these variables are only used in that branch.
| } default_litlen_costs[] = { | ||
| { /* match_prob = 0.25 */ | ||
| .used_lits_to_lit_cost = { | ||
| { |
Copilot
AI
Nov 25, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The removal of designated initializers (.used_lits_to_lit_cost = {) is necessary for C++ compatibility since designated initializers for nested structures are a C99 feature not fully supported in C++. However, this reduces code readability. Consider adding a comment explaining that this array initialization order must match the struct definition in the corresponding header file.
| if (cur != &(ctxt->first_part)) { dofree (cur); } | ||
| else { memset (cur, 0, sizeof (struct _priv_exr_part_t)); } | ||
| else | ||
| { |
Copilot
AI
Nov 25, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] In C++, the default constructor will initialize the struct members, but in C mode at line 193, memset is used. Consider documenting that these two initialization paths must produce equivalent results, as any mismatch could lead to subtle bugs.
| { | |
| { | |
| /* | |
| * IMPORTANT: The following two initialization paths (C++ and C) | |
| * must produce equivalent results. The struct _priv_exr_part_t | |
| * should remain a POD type with all members defaulting to zero. | |
| * If any member is added with a non-zero default, both branches | |
| * must be updated accordingly. Any mismatch could lead to subtle bugs. | |
| */ |
| return EXR_UNLOCK_WRITE_AND_RETURN (ctxt->print_error ( \ | ||
| ctxt, EXR_ERR_INVALID_ARGUMENT, "NULL output for '%s'", name)); \ | ||
| *out = attr->entry; \ | ||
| *out = EXR_CAST_TO_TYPE_OF(*out) attr->entry; \ |
Copilot
AI
Nov 25, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The macro EXR_CAST_TO_TYPE_OF uses decltype for MSVC C++ and __typeof__ for GCC/Clang. This adds a space after the cast in C mode which changes the semantics slightly. The empty definition for C mode should be verified to not cause issues with operator precedence in complex expressions.
When
OPENEXR_CORE_USE_NAMESPACE=ON, the .c and .h files in OpenEXRCore are built with C++ rather than C, with all symbols in theOPENEXR_IMF_INTERNAL_NAMESPACEnamespace just like the OpenEXR library.This makes is possible to trade the benefits of the C implementation for the benefits of C++ and the namespacing of all symbols.
All code in the OpenEXRCore .c and .h files is now bracketed inside
OPENEXR_CORE_NAMESPACE_ENTERandOPENEXR_CORE_NAMESPACE_EXIT, which add namespace directives (e.g.namepace Imf_3_4 {). These degenerate to empty when compiling with C, which is still the default.There are also macros
OPENEXR_CORE_EXTERN_C_ENTER/OPENEXR_CORE_EXTERN_C_EXITfor the OpenEXRCore code that needsextern "C" {, which reverts the the C++ OpenEXR namespace in C++.This also adds missing C++ casts to OpenEXRCore, required by C++ even though the are not necessary in C.
Also, the
_priv_exr_part_tstruct now has a custom constructor and assignment operator because itschunk_tablefield is aatomic_uintptr_t, which has those methods deleted.This also includes some necessary changes to the vendored
libdeflatecode to compile in C++.When building OpenEXR with the default configuration, this produces the same object code as before. The namespaced symbols are only generated when configured to do so. As as test, I ran
nmon the shared library and got the same symbols as before.Website preview: https://openexr--2205.org.readthedocs.build/en/2205/