libmach: update API again, factors out init/update/deinit from native #423

2022-07-20T03:45:44Z

zack466 commented

2022-07-20 03:45:44 +00:00

(Migrated from github.com)

This PR makes a few changes to the libmach API in accordance with the comments on the previous PR.

Also, it seems like I forgot to stage my changes to build.zig in the previous commit, so they are included in this commit instead.

By selecting this checkbox, I agree to license my contributions to this project under the license(s) described in the LICENSE file, and I have the right to do so or have received permission to do so by an employer or client I am producing work for whom has this right.

This PR makes a few changes to the libmach API in accordance with the comments on the previous PR. Also, it seems like I forgot to stage my changes to `build.zig` in the previous commit, so they are included in this commit instead. - [X] By selecting this checkbox, I agree to license my contributions to this project under the license(s) described in the LICENSE file, and I have the right to do so or have received permission to do so by an employer or client I am producing work for whom has this right.

emidoots (Migrated from github.com) reviewed

2022-07-20 16:21:36 +00:00

libmach/test.c

				`@ -4,13 +4,19 @@`

I wonder if it might be a good idea to have these prefixed with an underscore, use a _Force32 to force the enum to be 32-bits (in case someone puts it somewhere expecting a fixed size), similar to how webgpu.h does it. We could also name this "Status" instead of "Return" so that there could be other error status codes in the future:

typedef enum MachStatus {
    MachStatus_Success = 0x00000000,
    MachStatus_Error = 0x00000001,
    MachStatus_Force32 = 0x7FFFFFFF
} MachStatus;

Maybe you have a different idea for error handling though?

I wonder if it might be a good idea to have these prefixed with an underscore, use a `_Force32` to force the enum to be 32-bits (in case someone puts it somewhere expecting a fixed size), similar to how [webgpu.h does it](https://github.com/webgpu-native/webgpu-headers/blob/64db862b70cee7a5a6dc89dc7d920a0300125f4a/webgpu.h#L150-L158). We could also name this "Status" instead of "Return" so that there could be other error status codes in the future: ```c typedef enum MachStatus { MachStatus_Success = 0x00000000, MachStatus_Error = 0x00000001, MachStatus_Force32 = 0x7FFFFFFF } MachStatus; ``` Maybe you have a different idea for error handling though?

emidoots (Migrated from github.com) reviewed

2022-07-20 16:25:01 +00:00

src/platform/native.zig

				`@ -587,60 +587,80 @@ pub const Platform = struct {`

should remove double comment here

emidoots (Migrated from github.com) reviewed

2022-07-20 16:28:28 +00:00

src/platform/native.zig

				`@ -587,60 +587,80 @@ pub const Platform = struct {`

I think this function shouldn't be exported.

Also, while snake_case is correct in C (and libmach.zig), here it is not. The function names in this file should be coreDeinit, coreInit, coreUpdate - quoting the Zig style guide:

Roughly speaking: camelCaseFunctionName, TitleCaseTypeName, snake_case_variable_name. More precisely:

Of course what you did in libmach.zig is correct, since those names must match C conventions as exported functions.

I think this function shouldn't be exported. Also, while `snake_case` is correct in C (and `libmach.zig`), here it is not. The function names in this file should be `coreDeinit`, `coreInit`, `coreUpdate` - quoting the [Zig style guide](https://ziglang.org/documentation/master/#toc-Names): > Roughly speaking: camelCaseFunctionName, TitleCaseTypeName, snake_case_variable_name. More precisely: Of course what you did in `libmach.zig` is correct, since those names must match C conventions as exported functions.

emidoots (Migrated from github.com) reviewed

2022-07-20 16:30:17 +00:00

src/platform/native.zig

				`@ -587,60 +587,80 @@ pub const Platform = struct {`

How about we pass the allocator into this function instead? Would eliminate these semi-confusing comments, and is a better approach to use generally anyway.

emidoots commented

2022-07-20 16:31:43 +00:00

(Migrated from github.com)

A few small suggestions, once those are addressed I'll merge. Looks great overall!

zack466 (Migrated from github.com) reviewed

2022-07-20 18:21:44 +00:00

src/platform/native.zig

				`@ -587,60 +587,80 @@ pub const Platform = struct {`

Ok. I wasn't sure if the pattern in Zig was to explicitly pass the same allocator into the deinit function. I figured that it might be annoying to have to keep the allocator around in order to deinit an object (but I guess with defer that's not too much of an issue).

Ok. I wasn't sure if the pattern in Zig was to explicitly pass the same allocator into the deinit function. I figured that it might be annoying to have to keep the allocator around in order to deinit an object (but I guess with `defer` that's not too much of an issue).

iddev5 (Migrated from github.com) reviewed

2022-07-20 18:29:32 +00:00

iddev5 (Migrated from github.com) left a comment

This is a single review of the overall PR. While the PR implements everything well in its own ways, I think some things are confusing.

Core is an actual module in mach. This means all C functions with mach_core should map directly to src/Core.zig
What we are currently doing is a bit of magic. We are actually mixing parts from Platform (src/platform/native.zig) and Core(src/core.zig). This can cause confusion to people who are looking at the zig API for docs, because in the long run thats what people would do -- try to replicate the zig examples in C comparing the function names.
I think both should be kept separate. Core itself can be perfectly used without needing the mach entry point, and I think libmach should respect it. What in my suggestion implies separating the current API into to mach_platform_* and mach_core_*.

I m not suggesting to do it now with this PR. Doing it slowly over time is fine. I m just suggesting that there should be some goal/rules in order to clear up the path ahead. Thoughts?

This is a single review of the overall PR. While the PR implements everything well in its own ways, I think some things are confusing. - Core is an actual module in mach. This means all C functions with ``mach_core`` should map directly to src/Core.zig - What we are currently doing is a bit of magic. We are actually mixing parts from Platform (src/platform/native.zig) and Core(src/core.zig). This can cause confusion to people who are looking at the zig API for docs, because in the long run thats what people would do -- try to replicate the zig examples in C comparing the function names. - I think both should be kept separate. Core itself can be perfectly used without needing the mach entry point, and I think libmach should respect it. What in my suggestion implies separating the current API into to ``mach_platform_*`` and ``mach_core_*``. I m not suggesting to do it now with this PR. Doing it slowly over time is fine. I m just suggesting that there should be some goal/rules in order to clear up the path ahead. Thoughts?

zack466 (Migrated from github.com) reviewed

2022-07-20 18:47:50 +00:00

libmach/test.c