Skip to content

Conversation

@mitchellh
Copy link
Contributor

@mitchellh mitchellh commented Mar 8, 2022

Two changes in here, both needed to get the tests passing in this PR.

At least 3 more passing tests!

  1. For inferred allocs with values known at comptime but not forced comptime (i.e. not in a comptime {} block), we were setting the incorrect type, causing cascading errors. We can now set the type to the bitcast type. The previous comment noted we can't do that due to constness, but @Vexu's recent make_const_ptr work fixes this.

  2. A mistake from my prior PR on sentinel array initialization, I was not adding the sentinel to the end of the array value for comptime. Bonus: this solidifies that adding the sentinel as the final op in the array_init_sent ZIR is correct.

Tests used during debugging/development are below. First two are the inferred alloc commit, last one is the sentinel bug. The real behavior tests exercise both at once.

test "inferred alloc with sentinel" {
    const ary = [_:0]u8{42};
    const ptr: [*:0]const u8 = &ary;
    try expect(ptr[1] == 0);
}

test "inferred alloc with sentinel at comptime" {
    comptime {
        const ary = [_:0]u8{42};
        const ptr: [*:0]const u8 = &ary;
        try expect(ptr[1] == 0);
    }
}

test "comptime sentinel array init" {
    comptime {
        var ptr: [*:0]const u8 = &[_:0]u8{1};
        try expect(ptr[1] == 0);
    }
}

The comment notes that we can't because of constness, but the recent
work by @Vexu lets us use the bitcast ty cause constness comes later.

The following tests pass from this:

```
test "A" {
    const ary = [_:0]u8{42};
    const ptr: [*:0]const u8 = &ary;
    try expect(ptr[1] == 0);
}

test "B" {
    comptime {
        const ary = [_:0]u8{42};
        const ptr: [*:0]const u8 = &ary;
        try expect(ptr[1] == 0);
    }
}
```
I didn't realize that the `array` value type has the sentinel on it.
@andrewrk
Copy link
Member

andrewrk commented Mar 9, 2022

Nice work!

Tests used during debugging/development are below. First two are the inferred alloc commit, last one is the sentinel bug. The real behavior tests exercise both at once.

For future reference, I would consider it an improvement if you took any behavior tests that test too much at once and meaningfully split them into multiple independent tests like this.

@andrewrk andrewrk merged commit 935d208 into ziglang:master Mar 9, 2022
@mitchellh mitchellh deleted the inferred-ty branch March 9, 2022 01:56
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.

2 participants