Skip to content

proposal: remove the <<| operator #19635

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
nektro opened this issue Apr 12, 2024 · 6 comments
Open

proposal: remove the <<| operator #19635

nektro opened this issue Apr 12, 2024 · 6 comments
Labels
proposal This issue suggests modifications. If it also has the "accepted" label then it is planned.
Milestone

Comments

@nektro
Copy link
Contributor

nektro commented Apr 12, 2024

  • in a normal bitwise left shift b must be comptime-known or have a type with log2 number of bits as a. this is not the case in <<| and confusing to code readers and newcomers.
  • there is no >>| to complement it and nearly no one has noticed.
  • in the Zig repo it is not used at other than in the compiler to implement it, and in the tests to ensure it works
  • I have not used it in any of my Zig code written in the past few years

for these reasons I propose we remove it.

@nektro
Copy link
Contributor Author

nektro commented Apr 12, 2024

its been brought to my attention the second point was unnecessary because >>= already does this behavior. imo the proposal still stands though

@nektro
Copy link
Contributor Author

nektro commented Apr 12, 2024

https://sourcegraph.com/search?q=context:global+lang:Zig+%3C%3C%7C&patternType=keyword&sm=0 only surfaces Zig itself and two outside cases that could use << instead.

@jedisct1
Copy link
Contributor

Never used it either.

@castholm
Copy link
Contributor

I don't have especially strong feelings for this operator, but I don't really see any big problems with it either.

x << y is equivalent to x * pow(2, y).
x << y when y >= @bitSizeOf(T) will always be an error and invoke safety-checked UB. Zig prefers handling "always error" cases via the type system whenever possible, and std.math.Log2Int(T) is the closest thing available at the moment, so for x << y, it makes sense to require y to be of type std.math.Log2Int(T). (If/when arbitrary range integers are implemented I suspect that @Int(0, @bitSizeOf(T) - 1) will be used for the rhs instead.)

x <<| y is equivalent to x *| pow(2, y).
x <<| y when y >= @bitSizeOf(T) is not an error and has a well-defined result, so it makes sense for y to also be of type T for unsigned integers. Apparently, currently it is also T for signed integers, which allows negative values on the rhs that will always invoke UB, so if it is not removed it would probably be sound to refine it to require the rhs to be unsigned.

If anything, my question would be why there isn't a matching x <<% y operator that is equivalent to x *% pow(2, y). If my math is right such an operator would always return 0 when y >= @bitSizeOf(T).

@castholm
Copy link
Contributor

castholm commented Apr 13, 2024

Interestingly, the behavior for <<| currently differs between comptime and runtime and has incorrect results when shifting by >= @bitSizeOf(T) at runtime:

const std = @import("std");
test {
    const a: i8 = comptime (@as(i8, 99) <<| @as(i8, 111));

    var b_lhs: i8 = 99;
    var b_rhs: i8 = 111;
    _ = &b_lhs;
    _ = &b_rhs;
    const b: i8 = b_lhs <<| b_rhs;

    try std.testing.expectEqual(a, b);
}
1/1 repro.test_0... expected 127, found -1
FAIL (TestExpectedEqual)

LLVM defines its llvm.sshl/ushl.sat.* intrinsics to return a poison value if the rhs is "equal to or larger than the integer bit width of the arguments", so I suspect that this is what is happening here.

@andrewrk andrewrk added the proposal This issue suggests modifications. If it also has the "accepted" label then it is planned. label Aug 16, 2024
@andrewrk andrewrk added this to the 0.15.0 milestone Aug 16, 2024
@andrewrk
Copy link
Member

Related: #7605

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
proposal This issue suggests modifications. If it also has the "accepted" label then it is planned.
Projects
None yet
Development

No branches or pull requests

4 participants