zig icon indicating copy to clipboard operation
zig copied to clipboard

Audit problematic `(bits + 7) / 8` pattern more and fix it more nicely

Open wooster0 opened this issue 1 year ago • 0 comments

This is a follow-up issue to this discussion: https://github.com/ziglang/zig/pull/15879#discussion_r1208557275

Here's an example of this possibly problematic pattern that is found in many places in the compiler and the std:

            const bytes = try self.readBytesNoEof((@typeInfo(T).Int.bits + 7) / 8);

The type of the bits integer is very relevant here. @typeInfo(T).Int.bits is of type u16. If the bits value could be 65535 (note the integer types with the greatest bit size are u65535/i65535) and the bit size of the bits int type is less than 17, there is a problem. The fix could be:

-            const bytes = try self.readBytesNoEof((@typeInfo(T).Int.bits + 7) / 8);
+            const bytes = try self.readBytesNoEof(@intCast(u16, (@as(u17, @typeInfo(T).Int.bits) + 7) / 8));

However @jacobly0 discovered a slightly nicer pattern which is std.math.divCeil(bits, 8). I think we might want to do that instead of (bits + 7) / 8 or @intCast(u16, (@as(u17, bits) + 7) / 8). I think it's easier to read.

So the goal is to replace the existing pattern with that and also fix more of these issues because I think there are more.

Here are two commits that fix some of these issues for more reference:

  • https://github.com/ziglang/zig/pull/15879/commits/4ac6e38021bded63ea78806f26c0d964e8dc235e
  • https://github.com/ziglang/zig/pull/15537/commits/3fa310d8414b6aa200460323ada8fbb4a3ee05b6

A blocker for this might be #16022 if we decide to do @divCeil(bits, 8) everywhere which would be even nicer.

wooster0 avatar Jun 13 '23 22:06 wooster0