Skip to content

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
Member

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
Member

It is better to directly refer to external symbols

@jinzhongjia
Copy link
Member

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/translate-c#92 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
Member

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
Member

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
Member

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
Member

I believe everything is perfect now

@jinzhongjia jinzhongjia merged commit 9980a9e into webui-dev:main Oct 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

2 participants