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

#1680 fix segmentation fault when appending None #1778

Merged

Conversation

semagnum
Copy link
Contributor

Fixes #1680

Summary

Changes to reference parameter to raise TypeError instead of crashing with a segmentation fault. Created test_append_none to prevent regression.

Copy link

linux-foundation-easycla bot commented Jul 10, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: semagnum / name: Spencer Magnusson (6114856)

@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 80.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 81.70%. Comparing base (c0e97b0) to head (6114856).
Report is 18 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1778      +/-   ##
==========================================
- Coverage   84.11%   81.70%   -2.41%     
==========================================
  Files         198      176      -22     
  Lines       22241    12318    -9923     
  Branches     4687     3028    -1659     
==========================================
- Hits        18709    10065    -8644     
+ Misses       2610     1717     -893     
+ Partials      922      536     -386     
Flag Coverage Δ
py-unittests 81.70% <80.00%> (-2.41%) ⬇️

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

Files Coverage Δ
...entimelineio-bindings/otio_serializableObjects.cpp 93.66% <100.00%> (-0.20%) ⬇️
tests/test_track.py 93.47% <66.66%> (-1.88%) ⬇️

... and 65 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 e10fdd2...6114856. Read the comment docs.

@ssteinbach
Copy link
Collaborator

Just to make sure I understand- because it was pointer before, you could pass the python None value, which would coerce to a nullptr and then cause a segfault when adding. By switching to a ref, pybind disallows nullptr/None and raises a TypeError?

Copy link
Collaborator

@ssteinbach ssteinbach left a comment

Choose a reason for hiding this comment

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

LGTM, good catch

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.

good solution, thanks

@meshula meshula merged commit ec2bc15 into AcademySoftwareFoundation:main Jul 17, 2024
56 checks passed
@ssteinbach ssteinbach added this to the Public Beta 18 milestone Jul 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Segmentation fault when appending None to Collection types
4 participants