Skip to content

Handle exceptions in C bindings#3246

Merged
mwestphal merged 7 commits into
f3d-app:masterfrom
bo-dani:dama/handle-exceptions-c-bindings
Jun 23, 2026
Merged

Handle exceptions in C bindings#3246
mwestphal merged 7 commits into
f3d-app:masterfrom
bo-dani:dama/handle-exceptions-c-bindings

Conversation

@bo-dani

@bo-dani bo-dani commented Jun 17, 2026

Copy link
Copy Markdown
Contributor

Describe your changes

As explained in the issue, some C bindings are not handling exceptions explicitly thrown by the library. This PR addresses this. I'm only catching specific exceptions based on this comment.

Issue ticket number and link if any

Implements #2763.

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 did not use AI to generate any of the content of that pull request
  • I used AI to generate code in that pull request, if yes please disclose which part of the code was generated and with which model.

Continuous integration

Please write a comment to run CI, eg: \ci fast.
See here for more info.

@bo-dani

bo-dani commented Jun 17, 2026

Copy link
Copy Markdown
Contributor Author

\ci fast

@github-actions

Copy link
Copy Markdown

❌ Invalid CI command "fast"

@mwestphal

Copy link
Copy Markdown
Member

❌ Invalid CI command "fast"

strange

@mwestphal

Copy link
Copy Markdown
Member

Thanks for the PR, feel free to flag as readcy for review when it is! :)

@bo-dani bo-dani marked this pull request as ready for review June 18, 2026 06:47
@bo-dani bo-dani requested a review from a team as a code owner June 18, 2026 06:47

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

Are you sure you checked all calls to the libf3d ? I'd have expected more exceptions.

@bo-dani

bo-dani commented Jun 18, 2026

Copy link
Copy Markdown
Contributor Author

Are you sure you checked all calls to the libf3d ? I'd have expected more exceptions.

I believe so, some bindings were already handling exceptions so I didn't have to do anything there. But here's the modules I checked:

  • camera_c_api -> camera_impl.cxx: no exceptions thrown
  • context_c_api: already done
  • engine_c_api: already done
  • image_c_api: already done
  • interactor_c_api: addressed by this PR.
  • options_c_api: addressed by this PR.
  • scene_c_api: already done
  • types_c_api: already done, although it's catching all exceptions, instead of explicit ones. I can change that if you prefer.
  • utils_c_api: addressed by this PR.
  • window_c_api: exceptions thrown are not exposed to C bindings (might be wrong here though).

@bo-dani bo-dani requested a review from mwestphal June 19, 2026 06:37
@mwestphal mwestphal requested a review from Meakk June 19, 2026 06:47
Comment thread c/interactor_c_api.cxx Outdated
Comment thread c/options_c_api.cxx

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

options.h not properly try/catch

@bo-dani bo-dani requested a review from mwestphal June 21, 2026 18:48
@github-actions

Copy link
Copy Markdown

You are modifying libf3d public API! ⚠️Please update bindings accordingly⚠️!
You can find them in their respective directories: c, python, java, webassembly.

Comment thread c/options_c_api.cxx
Comment thread c/options_c_api.cxx Outdated
Comment thread c/options_c_api.cxx
Comment thread c/options_c_api.cxx

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

small changes needed

@bo-dani bo-dani requested a review from mwestphal June 22, 2026 07:13
Comment thread library/public/interactor.h 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.

one last change

@bo-dani bo-dani requested a review from mwestphal June 23, 2026 06:59
@mwestphal mwestphal added ci:full and removed ci:fast labels Jun 23, 2026
@mwestphal mwestphal self-requested a review June 23, 2026 07:46
@codecov

codecov Bot commented Jun 23, 2026

Copy link
Copy Markdown

Codecov Report

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

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #3246   +/-   ##
=======================================
  Coverage   97.06%   97.06%           
=======================================
  Files         213      212    -1     
  Lines       17656    17570   -86     
=======================================
- Hits        17137    17054   -83     
+ Misses        519      516    -3     

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

@mwestphal mwestphal merged commit 9d00f24 into f3d-app:master Jun 23, 2026
77 checks passed
@mwestphal

Copy link
Copy Markdown
Member

Thanks a lot for your contribution (and persistence!) @bo-dani

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants