Classify empty environment variables as unset #22497
Conversation
Modifies EnvVar.isSet() to assume empty strings are unset. This is required for correctly parsing NO_COLOR values, and @ifreund suggested that all variables should follow this behavior. Closes ziglang#22380.
sno2
force-pushed
the
fix-color-parsing
branch
from
458038d to
185f9cf
Compare
sno2
changed the title
| pub fn isSet(ev: EnvVar, arena: std.mem.Allocator) !bool { | ||
| const value = try ev.get(arena) orelse return false; | ||
| return value.len != 0; | ||
| } |
There was a problem hiding this comment.
It feels a little silly to be allocating just to check if it has a non-zero length. It's not a huge deal given the places this is called in, but it does make me feel like the std.process API could use some enhancements in the future.
There was a problem hiding this comment.
I agree. It would be great to have a version of std.process.getEnvVarOwned that includes a boolean if an allocation was required.
There was a problem hiding this comment.
It is Indeed silly that checking if an environment variable has a zero-length value using the cross-platform std.process API requires allocation.
Nonetheless, I think this patch makes the behavior of the Zig compiler more predictable and in line with established conventions. Improving the std.process API to save some allocations would be nice, but that can be done as a follow-up.
Modifies EnvVar.isSet() to assume empty strings are unset. This is required for correctly parsing NO_COLOR values, and ifreund suggested that all variables should follow this behavior.
Closes #22380.