- Notifications
You must be signed in to change notification settings - Fork 30
Refactor: Bindings simplification #68
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
Conversation
| Good job |
| It is better to directly refer to external symbols |
| Can you explain why you re-integrated the binding part into a source file? |
For what reason is it better to directly use the |
I can move the bindings back into a separate |
No, no, I mean what you did was right. |
| Now, I only have one question, should we split the source code? Like:
Just like the file structure of the current main branch |
Unfortunately we are not able to create a separate file for I'll push a change in just a moment with the |
| Awesome, thanks for your work |
| I've just split all the |
| I believe everything is perfect now |
I first noticed certain problems with the current Zig bindings when attempting to build for Windows using
-Dtarget=x86_64-windows-msvc.Notably, I encountered this issue due to
webuiheader. As such, while reviewing thewebuiheader import, I noticed the second problem; the use ofusingnamespace, which is strongly recommended against by Zig core contributors.While digging further into the current Zig bindings implementation, I noticed several more problems, chiefly including the duplication of the
Eventstruct.As such, I decided to simplify the bindings implementation.
Through this rework, the following is achieved:
x86_64-windows-msvcstill does not work due to a separate issue)webuiC functions with Zig-ified signaturesEventstruct de-duplicationThis rework largely maintains an identical public interface, with the following caveats:
Eventstruct must now be passed as a pointer to the vast majority of functionsdestorytypo changed todestroyThis rework also brings to light a potential bug in the previous bindings implementation: the previous
webui_mallocuse did not check for allocation failure. However, addressing this issue in the public interface would require a change to the public function signature, and thus it was left as aTODOfor future consideration.While there were no tests I could run, all examples built successfully and several privately developed applications also built/executed as previous.