marlersoft / zigwin32

Zig bindings for Win32 generated by https://github.com/marlersoft/zigwin32gen
MIT License
234 stars 30 forks source link

Null pointer usage? #2

Closed voltflake closed 2 years ago

voltflake commented 3 years ago

I used WriteConsoleA but I'm sure many win32 functions have this problem. I'm still learning ZIG. Correct me where I'm wrong.

My code example.

const console = @import("win32.zig").system.console;

pub fn main() anyerror!void {
    _ = console.WriteConsoleA(
        console.GetStdHandle(console.STD_OUTPUT_HANDLE),
        "All your codebase are belong to us.",
        "All your codebase are belong to us.".len,
        null,
        null);    // Should it be used like this?
}

WriteConsoleA function declaration.

pub extern "KERNEL32" fn WriteConsoleA(
    hConsoleOutput: HANDLE,
    lpBuffer: [*]const u8,
    nNumberOfCharsToWrite: u32,
    lpNumberOfCharsWritten: ?*u32,
    lpReserved: *c_void,
) callconv(@import("std").os.windows.WINAPI) BOOL;

I think lpReserved: *c_void, shold be ?*c_void to allow null values. lpReserved parameter should always be null according to the official documentation.

marler8997 commented 3 years ago

I've been using the "Optional" SAL attribute to determine whether or not pointers can be NULL. However, the metadata seems to be missing this attribute in many places for pointers that can be NULL, including struct fields (see https://github.com/microsoft/win32metadata/issues/523).

For now I've been adding "patches" to the metadata that add optional when it's missing (i.e. https://github.com/marlersoft/win32jsongen/blob/ca90c05775ae1c2eb009013683799278bd5ad62a/Generator/Patch.cs#L27)

I'm not sure how many of these optionals are actually missing though. In the meantime I can add a patch for this and I've opened an issue in the metadata for this particular case here: https://github.com/microsoft/win32metadata/issues/567

marler8997 commented 3 years ago

Patch added here: https://github.com/marlersoft/win32jsongen/commit/c8699e3570be1071b1d05e7c99eeab3911e741e7#diff-9b91f2051020d9722023425e036feb5d36cfb29e6b6c59e68b3f7429df50ca21R36

zigwin32 regnerated commit here: https://github.com/marlersoft/zigwin32/commit/2cde15a78def045bfae704ea39887c525bacac17

marler8997 commented 2 years ago

By the way, based on the metadata repo's response that they won't be supporting data that indicates whether or not pointers can be null, I've reworked the Zig projection. The new approach is to make all pointers nullable by default. To declare a pointer as being "required" or "notnull", I've add a file named "notnull.json" where we can add function parameters (and probably struct fields later) that cannot be null. This approach means that the API will just work for projects, and if they want more type correctness we can add it later on. This is in contrast to the previous approach where certain functions/types were unusable for projects because they were unable to pass NULL to pointers that were supposed to accept it.

The commit to implement this change is here: https://github.com/marlersoft/zigwin32gen/commit/b63c76ee2f070dbb3cb027484c194f61316b9eb2

The corresponding difference in the bindings is here: https://github.com/marlersoft/zigwin32/commit/955e91593f8eb1eeae4b51bf199dbdaef8365c94

It only affected 117,526 lines...lol :)

With this I"ll consider the matter solved, let me know if you still have issues.