Skip to content

Conversation

@HydroH
Copy link
Contributor

@HydroH HydroH commented Nov 27, 2024

Closes #19949 .

@alexrp alexrp enabled auto-merge (rebase) November 27, 2024 18:11
@squeek502
Copy link
Member

squeek502 commented Nov 27, 2024

I'm not sure these changes make sense. All of these are out parameters, meaning the null terminator is written by the function, so the input being null-terminated is irrelevant. Further, in terms of usage, I think it makes more sense for these parameters to not be defined as null-terminated.

For example, if these parameters are defined with a null-terminator, the correct usage would look something like this:

var wtf16le_buf: [256:0]u16 = undefined;
// Have to pass len + 1 since the NUL-terminator should be included in the buffer length
const result = std.os.windows.kernel32.GetCurrentDirectoryW(wtf16le_buf.len + 1, &wtf16le_buf);

if (result == 0) return error.Foo;

// GetCurrentDirectoryW returns the length not including the terminator
const cur_dir = wtf16le_buf[0..result :0];

If it's not defined with a null terminator, the usage is more intuitive since just passing the length of the buffer is correct:

var wtf16le_buf: [256]u16 = undefined;
const result = std.os.windows.kernel32.GetCurrentDirectoryW(wtf16le_buf.len, &wtf16le_buf);

if (result == 0) return error.Foo;

// GetCurrentDirectoryW returns the length not including the terminator
const cur_dir = wtf16le_buf[0..result :0];

So, the buffer being defined as NUL-terminated doesn't gain us anything and makes it less likely that the correct length of the buffer will be passed to the function.

(I've made this same point before on this PR, but wasn't aware of #19949)


This comment is also relevant:

I believe this lpBuffer change [for GetEnvironmentVariableW] would actually cause a compile error in std.os.windows.GetEnvironmentVariableW, but since it's not used anywhere, it's not detected (we have std.process.getenvW that serves the same purpose).

@andrewrk andrewrk disabled auto-merge November 27, 2024 19:13
@alexrp
Copy link
Member

alexrp commented Nov 27, 2024

Good catch, I hadn't considered that nuance. I think I agree with your reasoning.

It does seem a bit unfortunate that the language feature designed specifically to represent this concept doesn't apply cleanly here, though. But I don't know that I have any concrete suggestions for improving that. 😕

@squeek502
Copy link
Member

I would argue that [*]u16 does apply fairly cleanly here. If there was to be a more accurate parameter type, it'd have to represent "is an arbitrary buffer that will become sentinel-terminated after this function returns." That sort of thing seems like the domain of doc comments, though.

@HydroH HydroH closed this Nov 28, 2024
@HydroH
Copy link
Contributor Author

HydroH commented Nov 28, 2024

I'm not sure these changes make sense. All of these are out parameters, meaning the null terminator is written by the function, so the input being null-terminated is irrelevant. Further, in terms of usage, I think it makes more sense for these parameters to not be defined as null-terminated.

For example, if these parameters are defined with a null-terminator, the correct usage would look something like this:

var wtf16le_buf: [256:0]u16 = undefined;
// Have to pass len + 1 since the NUL-terminator should be included in the buffer length
const result = std.os.windows.kernel32.GetCurrentDirectoryW(wtf16le_buf.len + 1, &wtf16le_buf);

if (result == 0) return error.Foo;

// GetCurrentDirectoryW returns the length not including the terminator
const cur_dir = wtf16le_buf[0..result :0];

If it's not defined with a null terminator, the usage is more intuitive since just passing the length of the buffer is correct:

var wtf16le_buf: [256]u16 = undefined;
const result = std.os.windows.kernel32.GetCurrentDirectoryW(wtf16le_buf.len, &wtf16le_buf);

if (result == 0) return error.Foo;

// GetCurrentDirectoryW returns the length not including the terminator
const cur_dir = wtf16le_buf[0..result :0];

So, the buffer being defined as NUL-terminated doesn't gain us anything and makes it less likely that the correct length of the buffer will be passed to the function.

(I've made this same point before on this PR, but wasn't aware of #19949)


This comment is also relevant:

I believe this lpBuffer change [for GetEnvironmentVariableW] would actually cause a compile error in std.os.windows.GetEnvironmentVariableW, but since it's not used anywhere, it's not detected (we have std.process.getenvW that serves the same purpose).

I think you are correct. I will close this PR. Maybe the issue should also get closed now?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Missing null terminator on various kernel32 signatures

3 participants