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

Offer alternative for cxxabi on Windows (Swift+Clang). #1787

Closed
wants to merge 1 commit into from

Conversation

furby-tm
Copy link
Contributor

@furby-tm furby-tm commented Jul 29, 2024

Summarize your change.

This fixes the clang compilation of OpenTimelineIO on Microsoft Windows, when compiling with Swift. Previously, the cxxabi.h header was erroneously getting included in the Windows clang compilation of OpenTimelineIO's stringUtils.cpp file because it was being conditionally compiled for clang across all platforms, and this header does not exist on the Windows platform.

  • The include for cxxabi.h is now guarded via __has_include(<cxxabi.h>).
  • A new preprocessor definition for OTIO_HAVE_DEMANGLER is set to 1 if cxxabi.h exists, and 0 if this header does not exist.
  • Previous checks against defined(__GNUC__) || defined(__clang__) are replaced with conditional compilation checks against the new OTIO_HAVE_DEMANGLER preprocessor define.

Copy link

linux-foundation-easycla bot commented Jul 29, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: furby-tm / name: furby™ (69144f2)

@codecov-commenter
Copy link

codecov-commenter commented Jul 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.71%. Comparing base (c0e97b0) to head (69144f2).
Report is 21 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1787      +/-   ##
==========================================
- Coverage   84.11%   81.71%   -2.41%     
==========================================
  Files         198      176      -22     
  Lines       22241    12319    -9922     
  Branches     4687     3022    -1665     
==========================================
- Hits        18709    10066    -8643     
+ Misses       2610     1717     -893     
+ Partials      922      536     -386     
Flag Coverage Δ
py-unittests 81.71% <ø> (-2.41%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
src/opentimelineio/stringUtils.cpp 44.44% <ø> (ø)

... and 67 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d213805...69144f2. Read the comment docs.

@furby-tm furby-tm force-pushed the main branch 2 times, most recently from 170b34c to 6ca724b Compare July 30, 2024 12:57
* This fixes the clang compilation of OpenTimelineIO on Windows, when compiling with Swift.

Signed-off-by: furby™ <[email protected]>
Copy link
Collaborator

@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.

Thanks for this contribution, this looks good to me.

I do have a question for the team, perhaps the cxxabi functions are rather overkill for this application. I'd prefer that we simplify this code to not do this demangling, I don't honestly think it's adding value.

@furby-tm
Copy link
Contributor Author

furby-tm commented Aug 3, 2024

If the team prefers to remove the cxxabi demangling, I can update this PR to deprecate it instead. Please feel free to ping me in this issue once the team has reached a consensus.

@reinecke reinecke added question Seeking guidance, clarification, or assistance. Not necessarily code related. cmake issues with the cmake scripts labels Aug 15, 2024
@jminor
Copy link
Collaborator

jminor commented Oct 1, 2024

From some offline discussion, it seems there is no objection to removing the demangling. @furby-tm we would gladly accept this PR as-is, or with the demangling removed. Please let us know which you'd like.

@furby-tm
Copy link
Contributor Author

furby-tm commented Oct 1, 2024

I'll go ahead and remove the demangling @jminor

@furby-tm
Copy link
Contributor Author

furby-tm commented Oct 1, 2024

Closing in favor of #1800

@furby-tm furby-tm closed this Oct 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cmake issues with the cmake scripts question Seeking guidance, clarification, or assistance. Not necessarily code related.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants