Skip to content
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

always allow integer comparison operations no matter the bit width, signedness, or comptime-ness of operands #2133

Closed
shawnl opened this issue Mar 29, 2019 · 12 comments
Labels
accepted This proposal is planned. proposal This issue suggests modifications. If it also has the "accepted" label then it is planned.
Milestone

Comments

@shawnl
Copy link
Contributor

shawnl commented Mar 29, 2019

This should compile:

var b: bool = undefined;
b = u8(1) > i8(1);

We should also consider and either accept, or explicitly reject (document) this:

var b: bool = undefined;
b = -u8(1) < i8(1);

The second case obeys our documented operator precedence, but in a very naïve implementation would need a i9 type here. I think this should be supported. If so, this support should to be explicitly documented by an example in the docs.


Supporting this is needed in order to make math.minInt and math.maxInt return T rather than comptime_int, which is desired by all the bit-manipulation intrinsics, as these return values have a fixed-length, unlike comptime_int.

@tgschultz
Copy link
Contributor

tgschultz commented Mar 29, 2019

I disagree for the same reason all other operators do not implicitly cast. consider:

const a = u8(255);
const b = i8(-1);
const c = a > b;

what is the expected value of c?

@shawnl
Copy link
Contributor Author

shawnl commented Mar 29, 2019

OK, then I will just fix all the types, and convert minInt and maxInt to return T instead of comptime_int.

@shawnl shawnl closed this as completed Mar 29, 2019
@shawnl
Copy link
Contributor Author

shawnl commented Mar 29, 2019

actualy, those aren't that useful to bit functions. so nothing

@shawnl
Copy link
Contributor Author

shawnl commented Aug 3, 2019

@tgschultz true. It would zext a by one bit, sext b by one bit, and then do a signed comparison.

See my PR #3001

@shawnl shawnl reopened this Aug 3, 2019
@daurnimator daurnimator added the proposal This issue suggests modifications. If it also has the "accepted" label then it is planned. label Aug 4, 2019
@andrewrk
Copy link
Member

andrewrk commented Aug 5, 2019

what is the expected value of c?

I think true is reasonable here since, mathematically, 255 > -1. Zig allows this with only implicit casts: comptime_int(u8(255)) > comptime_int(i8(-1))

In Zig integers represent their mathematical values. There are a few design decisions that reflect this:

  • integer overflow (unless using special operators) is undefined behavior
  • @intCast is undefined behavior if the mathematical value of the integer is not preserved
  • implicit integer casts are allowed when the types guarantee the mathematical value will be preserved

Further we already have precedence for relaxing type errors when the value is comptime known:

    const a: i64 = 10;
    const b: u8 = a; //ok

@andrewrk andrewrk added this to the 0.6.0 milestone Aug 5, 2019
@andrewrk andrewrk changed the title > and < should allow safe comparisons between signed and unsigned types allow integer comparison operations with signed and unsigned operands when both are comptime known Aug 5, 2019
@andrewrk andrewrk added the accepted This proposal is planned. label Aug 5, 2019
@andrewrk
Copy link
Member

andrewrk commented Aug 5, 2019

This test case already passes:

comptime expect(255 > -1);

This test case is accepted:

comptime expect(u8(255) > i8(-1));

This test case is not accepted:

const b = -u8(1) < i8(1); // error: invalid negation type: 'u8'

@kyle-github
Copy link

@andrewrk so the comparison is allowed but only if the values are comptime known? That seems to make sense with the existing automatic comptime coercion allowed. Sorry if that was obvious, but I only just noticed that the title of the issue changed :-(

Should that last test case pass?

@shawnl
Copy link
Contributor Author

shawnl commented Aug 6, 2019

Is this also accepted for runtime values?

@andrewrk
Copy link
Member

andrewrk commented Aug 6, 2019

If one of the operands is runtime known, then the comptime operand is casted to the type of the runtime operand.

var a: u8 = 255;
const b: i8 = 10;
expect(a > b); // OK

const c: u8 = 255;
var d: i8 = 10;
expect(c > d); // error. cannot cast 255 to i8

another case: if the operation can be comptime known without any casting, it also works:

var a: u8 = 255;
const b: i8 = -10;
comptime expect(a > b); // OK

@shawnl
Copy link
Contributor Author

shawnl commented Aug 6, 2019

This can also work if both operands are runtime.

@andrewrk
Copy link
Member

Another use case of this:

        pub fn emitNumber(
            self: *Self,
            /// An integer, float, or `std.math.BigInt`. Emitted as a bare number if it fits losslessly
            /// in a IEEE 754 double float, otherwise emitted as a string to the full precision.
            value: var,
        ) !void {
            assert(self.state[self.state_index] == State.Value);
            switch (@typeInfo(@typeOf(value))) {
                .Int => if (value < 4503599627370496 and value > -4503599627370496) {
                    try self.stream.print("{}", value);
                    self.popState();
                    return;
                },
                .Float => if (@floatCast(f64, value) == value) {
                    try self.stream.print("{}", value);
                    self.popState();
                    return;
                },
                else => {},
            }
            try self.stream.print("\"{}\"", value);
            self.popState();
        }
/home/andy/dev/zig/lib/std/json/write_stream.zig:154:37: error: integer value 4503599627370496 cannot be implicitly casted to type 'i32'
                .Int => if (value < 4503599627370496 and value > -4503599627370496) {
                                    ^

shawnl added a commit to shawnl/zig that referenced this issue Nov 5, 2019
It is a deviation from C, but I think we should consider making this the behavior
of the operators. See ziglang#2133
@andrewrk andrewrk changed the title allow integer comparison operations with signed and unsigned operands when both are comptime known always allow integer comparison operations no matter the bit width or signedness of operands Dec 10, 2019
@andrewrk
Copy link
Member

@shawnl looks like I did not understand the point of this proposal before; I do now, and it is planned.

@andrewrk andrewrk changed the title always allow integer comparison operations no matter the bit width or signedness of operands always allow integer comparison operations no matter the bit width, signedness, or comptime-ness of operands Dec 10, 2019
andrewrk pushed a commit that referenced this issue Dec 16, 2019
It is a deviation from C, but I think we should consider making this the behavior
of the operators. See #2133
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted This proposal is planned. 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

5 participants