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

Refactor: Bindings simplification #68

Merged
merged 3 commits into from
Oct 8, 2024
Merged

Conversation

mochalins
Copy link
Contributor

@mochalins mochalins commented Oct 8, 2024

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 webui header. As such, while reviewing the webui header import, I noticed the second problem; the use of usingnamespace, 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 Event struct.

As such, I decided to simplify the bindings implementation.

Through this rework, the following is achieved:

  1. Avoiding Zig issue 20065 as referenced above (although building on x86_64-windows-msvc still does not work due to a separate issue)
  2. Direct exposure of webui C functions with Zig-ified signatures
  3. Simplification of codebase with minimal layers of abstraction
  4. Reduction of memory usage with Event struct de-duplication

This rework largely maintains an identical public interface, with the following caveats:

  1. Enum names and backing types slightly adjusted to better match Zig naming standards/underlying C tag type
  2. Event struct must now be passed as a pointer to the vast majority of functions
  3. destory typo changed to destroy

This rework also brings to light a potential bug in the previous bindings implementation: the previous webui_malloc use 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 a TODO for future consideration.

While there were no tests I could run, all examples built successfully and several privately developed applications also built/executed as previous.

@jinzhongjia
Copy link
Collaborator

Good job
But please wait for me to take a closer look if necessary.
I've taken a quick look at it, and indeed, using usingnamespce is not a good habit.
Officials are considering deprecating this feature

@jinzhongjia
Copy link
Collaborator

It is better to directly refer to external symbols

@jinzhongjia
Copy link
Collaborator

Can you explain why you re-integrated the binding part into a source file?
From my personal point of view, I think putting the source code in one file will make the entire binding less readable.
When I originally created this binding, I had only one source file. Later, when I followed the upstream changes, I felt uncomfortable flipping through one file.

@mochalins
Copy link
Contributor Author

It is better to directly refer to external symbols

For what reason is it better to directly use the cImport signatures? As far as I know, most well-established bindings (including bindings in the Zig standard library, e.g. https://github.com/ziglang/zig/blob/master/lib/std/os/windows/kernel32.zig#L71) redefine the C function in a slightly-safer Zig signature. This also avoids ziglang/zig#20065 as mentioned above, and is generally considered best-practice to avoid cImport's automatic C translation (auto-C-translation still has quite a few issues, especially around usage of macros).

@mochalins
Copy link
Contributor Author

mochalins commented Oct 8, 2024

Can you explain why you re-integrated the binding part into a source file? From my personal point of view, I think putting the source code in one file will make the entire binding less readable. When I originally created this binding, I had only one source file. Later, when I followed the upstream changes, I felt uncomfortable flipping through one file.

I can move the bindings back into a separate header.zig file if desired, but I personally felt that directly exposing them through the webui module -- e.g. webui.webui_wait() -- was a simpler experience for end users. No strong preference either way.

@jinzhongjia
Copy link
Collaborator

Can you explain why you re-integrated the binding part into a source file? From my personal point of view, I think putting the source code in one file will make the entire binding less readable. When I originally created this binding, I had only one source file. Later, when I followed the upstream changes, I felt uncomfortable flipping through one file.

I can move the bindings back into a separate header file if desired, but I personally felt that directly exposing them through the webui module -- e.g. webui.webui_wait() -- was a simpler experience for end users. No strong preference either way.

No, no, I mean what you did was right.

@jinzhongjia
Copy link
Collaborator

Now, I only have one question, should we split the source code?

Like:

  • a file for "direct exposure of webui C functions with Zig-ified signatures"
  • a file for type definition
  • a file for webui self
  • a file for Event

Just like the file structure of the current main branch

@jinzhongjia jinzhongjia changed the title Bindings simplification Refactor: Bindings simplification Oct 8, 2024
@mochalins
Copy link
Contributor Author

Now, I only have one question, should we split the source code?

Like:

* a file for "direct exposure of webui C functions with Zig-ified signatures"

* a file for type definition

* a file for webui self

* a file for `Event`

Just like the file structure of the current main branch

Unfortunately we are not able to create a separate file for Event because files cannot be defined as extern structs, only normal structs. However, I can separate all the extern fns into a separate file; I think the type definitions are better suited in webui, as users may want to directly forward the types (e.g. webui.Browser) in applications.

I'll push a change in just a moment with the extern fns separated!

@jinzhongjia
Copy link
Collaborator

Awesome, thanks for your work

@jinzhongjia jinzhongjia added the enhancement New feature or request label Oct 8, 2024
@mochalins
Copy link
Contributor Author

I've just split all the extern functions into a separate module called c! I've called the module c to clearly indicate to users that all the functions contained there are the raw C bindings to webui, but if you feel that a different name would be more appropriate please let me know.

@jinzhongjia
Copy link
Collaborator

I believe everything is perfect now

@jinzhongjia jinzhongjia merged commit 9980a9e into webui-dev:main Oct 8, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants