-
Notifications
You must be signed in to change notification settings - Fork 306
Export C++ symbols #1920
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?
Export C++ symbols #1920
Conversation
Signed-off-by: Darby Johnston <[email protected]>
Codecov Report✅ All modified and coverable lines are covered by tests. ❌ Your changes status has failed because you have indirect coverage changes. Learn more about Unexpected Coverage Changes and reasons for indirect coverage changes. Additional details and impacted files@@ Coverage Diff @@
## main #1920 +/- ##
==========================================
+ Coverage 84.11% 84.86% +0.74%
==========================================
Files 198 181 -17
Lines 22241 13054 -9187
Branches 4687 1206 -3481
==========================================
- Hits 18709 11078 -7631
+ Misses 2610 1790 -820
+ Partials 922 186 -736
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 114 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
LGTM. I wonder if |
Signed-off-by: Darby Johnston <[email protected]>
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.
Couple of questions! Thanks for putting this together though. Did you check to see if the DSO sizes or load times change at all after this?
src/opentime/rationalTime.h
Outdated
OPENTIME_EXPORT constexpr double | ||
fabs(double val) noexcept |
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.
Do we need to export the fabs
symbol?
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.
Also, @darbyjohnston I suspect this is a question for another PR, but is this still true?
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.
Good point, we should probably remove it from the public API entirely. @meshula thought it was a worthwhile optimization, and when we upgrade to C++ 23 we can replace it with the std library version.
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.
Agreed - @meshula let us know if you think it should stay in this public header and be exported
I checked and the sizes were the same, but I exported all of the symbols so I didn't expect any difference. Hopefully it's a feature we can take more advantage of in the future, for example the C++ OTIOZ PR has some code that might not need to be part of the public API. |
Co-authored-by: Stephan Steinbach <[email protected]> Signed-off-by: Darby Johnston <[email protected]>
Signed-off-by: Darby Johnston <[email protected]>
Fixes #1919
This change uses the CMake GenerateExportHeader functionality to explicitly export C++ symbols. In addition to fixing #1919, this can also potentially make the use of OTIO shared libraries more efficient.