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

Add nullptr guard-check in comparision of shared ptr node and edge #436

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

badumbatish
Copy link
Contributor

Fixes Clang's critical bug on the library where we're doing custom shared_ptr comparison with potentially null ptrs.

This will fix #433 but i think what we should also be doing is enforce nullptr safety on these custom operators on shared ptrs

This allows Clang on MacOS to pass all tests.

  • A description of the changes proposed in the pull request.
    I added
...
  if (p1 == nullptr && p2 == nullptr) return true;
  if (p1 == nullptr || p2 == nullptr) return false;
...

to == operator in PointerHash()

@ZigRazor would love a review

Copy link

codecov bot commented May 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.87%. Comparing base (1b31e63) to head (80e69fd).
Report is 25 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #436      +/-   ##
==========================================
+ Coverage   97.58%   97.87%   +0.28%     
==========================================
  Files          87       87              
  Lines        9492    10063     +571     
  Branches        0      670     +670     
==========================================
+ Hits         9263     9849     +586     
+ Misses        229      214      -15     
Flag Coverage Δ
unittests 97.87% <100.00%> (+0.28%) ⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Owner

@ZigRazor ZigRazor left a comment

Choose a reason for hiding this comment

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

Good one! Thank you so much!

@ZigRazor
Copy link
Owner

We are waiting the re-liensing #427, after that we can merge it!

@ZigRazor ZigRazor self-assigned this May 13, 2024
@ZigRazor ZigRazor added bug Something isn't working enhancement New feature or request macos Macos related labels May 13, 2024
@badumbatish
Copy link
Contributor Author

how should i pass the rest of the tests on ci/cd ? @ZigRazor

@ZigRazor
Copy link
Owner

how should i pass the rest of the tests on ci/cd ? @ZigRazor

The only one you can pass is the clang format, just formatting with clang. @badumbatish
The ore CI/CD should be fix, now aren't working

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request macos Macos related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Test failed on MacOS?
3 participants