Skip to content

C image bindings: Add null pointer checks#3394

Merged
Meakk merged 9 commits into
f3d-app:masterfrom
SahilSonar-04:null-check-image-c-binding
Jul 20, 2026
Merged

C image bindings: Add null pointer checks#3394
Meakk merged 9 commits into
f3d-app:masterfrom
SahilSonar-04:null-check-image-c-binding

Conversation

@SahilSonar-04

@SahilSonar-04 SahilSonar-04 commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

Added Null Pointer checks for image c bindings

Fixes Issue : #3393

Checklist for finalizing the PR

  • I have performed a self-review of my code
  • I have added tests for new features and bugfixes
  • I have added documentation for new features
  • If it is a modifying the libf3d API, I have updated bindings
  • If it is a modifying the .github/workflows/versions.json, I have updated docker_timestamp

AI Disclosure

  • I have not used AI to generate any of the content of this pull request
  • I have used AI to generate code in this pull request:
  • I have carefully read and understood the AI policy.
  • I have carefully reviewed and completely understood every generated line.
  • I disclose below which parts of the code were generated and with which AI model:

@SahilSonar-04
SahilSonar-04 requested a review from a team as a code owner July 17, 2026 19:33
@mwestphal

Copy link
Copy Markdown
Member

Please properly disclose AI usage. Check only the correct checkbox. In the top part too.

@mwestphal mwestphal left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Improper AI disclosure.

@mwestphal

Copy link
Copy Markdown
Member

Thanks for fixing it @SahilSonar-04 !

@mwestphal mwestphal left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks fine however I think that:

  • (almost) each of these should print an error
  • all the docs should be updated

Comment thread c/image_c_api.cxx Outdated
Comment thread c/image_c_api.cxx
Comment thread c/image_c_api.cxx
@SahilSonar-04

Copy link
Copy Markdown
Contributor Author

Looks fine however I think that:

  • (almost) each of these should print an error
  • all the docs should be updated

Wouldn't these cause inconsistencies ?
As other c binding files rn follow the same pattern

@mwestphal

Copy link
Copy Markdown
Member

As other c binding files rn follow the same pattern

Maybe I'm wrong about printing an error, thats why I asked for @Meakk input.

However the doc defintely should mention this behavior, if it does not, it is missing.

@SahilSonar-04

Copy link
Copy Markdown
Contributor Author

Done with the documentation , Do let me know if any changes required , also about the error messages

Comment thread c/image_c_api.cxx Outdated
Comment thread c/image_c_api.cxx
@SahilSonar-04
SahilSonar-04 requested review from Meakk and mwestphal July 19, 2026 13:27
Comment thread c/image_c_api.h Outdated
Comment thread c/image_c_api.h Outdated
Comment thread c/image_c_api.h Outdated
Comment thread c/image_c_api.h Outdated
Comment thread c/image_c_api.h Outdated
Comment thread c/image_c_api.h Outdated
Comment thread c/image_c_api.h Outdated
Comment thread c/image_c_api.h Outdated
Comment thread c/image_c_api.h Outdated
Comment thread c/image_c_api.h Outdated
Comment thread c/image_c_api.h Outdated
Comment thread c/image_c_api.h Outdated
Comment thread c/image_c_api.h Outdated
Comment thread c/image_c_api.h Outdated
Comment thread c/image_c_api.h Outdated
Comment thread c/image_c_api.cxx Outdated

@mwestphal mwestphal left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

changes needed

@SahilSonar-04
SahilSonar-04 requested a review from mwestphal July 20, 2026 05:13
@Meakk

Meakk commented Jul 20, 2026

Copy link
Copy Markdown
Member

\ci fast

Comment thread c/image_c_api.h Outdated
Comment thread c/image_c_api.cxx Outdated
Comment thread c/image_c_api.cxx Outdated

@mwestphal mwestphal left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

other small changes, also CI is red

@SahilSonar-04
SahilSonar-04 requested a review from mwestphal July 20, 2026 09:05
@SahilSonar-04

Copy link
Copy Markdown
Contributor Author

\ci extended

@SahilSonar-04

Copy link
Copy Markdown
Contributor Author

\ci full

@codecov

codecov Bot commented Jul 20, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.08%. Comparing base (e236322) to head (ce47b01).
⚠️ Report is 3 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #3394   +/-   ##
=======================================
  Coverage   97.08%   97.08%           
=======================================
  Files         216      216           
  Lines       18750    18771   +21     
=======================================
+ Hits        18203    18224   +21     
  Misses        547      547           

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@Meakk
Meakk merged commit 84009bc into f3d-app:master Jul 20, 2026
94 checks passed
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.

3 participants