Stage2: more optionals stuff #6060

Vexu · 2020-08-15T17:44:21Z

No description provided.

andrewrk

Nice, this is substantial progress :)

andrewrk · 2020-08-15T18:00:49Z

src-self-hosted/astgen.zig

@@ -452,6 +453,12 @@ fn boolNot(mod: *Module, scope: *Scope, node: *ast.Node.SimplePrefixOp) InnerErr
    return addZIRUnOp(mod, scope, src, .boolnot, operand);
 }

+fn addressOf(mod: *Module, scope: *Scope, node: *ast.Node.SimplePrefixOp) InnerError!*zir.Inst {
+    const tree = scope.tree();
+    const src = tree.token_locs[node.op_token].start;


unused tree and src

andrewrk · 2020-08-15T18:21:10Z

src-self-hosted/astgen.zig

+        node.ptr_info.sentinel == null;
+
+    if (simple) {
+        const child_type = try expr(mod, scope, .{ .ty = meta_type }, node.rhs);


Looks like this line and 494 can be moved up to above this branch

I was planning on putting 494 after all the other attributes so that they would get analyzed in the order that they appear in the source but I couldn't figure how to pass child_type to the sentinel cast result loc. If the order is not important then I'll just move it before the shortcut.

Ahh that makes sense. Never mind my review comment then!

That would still depend on being able to refer to the child type before the sentinel which doesn't seem possible but all the other attributes before the child type would still be an improvement.

andrewrk · 2020-08-15T18:27:59Z

src-self-hosted/astgen.zig

+        node.ptr_info.volatile_token == null and
+        node.ptr_info.sentinel == null;
+
+    if (simple) {


I like how you did this 👍

andrewrk · 2020-08-15T18:33:49Z

src-self-hosted/type.zig

@@ -107,6 +107,17 @@ pub const Type = extern union {
        return @fieldParentPtr(T, "base", self.ptr_otherwise);
    }

+    pub fn castPointer(self: Type) ?*Payload.Pointer {


It looks like Type.zig and Value.zig might benefit from being reworked into following the pattern set by std.zig.ast, ZIR, and IR with the castTag function (and the preference to use that function rather than the one that accepts the type parameter). This pattern has been working nicely elsewhere to make multiple tags map to the same payload type.

andrewrk · 2020-08-15T18:40:19Z

src-self-hosted/zir_sema.zig

-        try mod.singleConstPtrType(scope, unwrap.base.src, child_type)
-    else
-        try mod.singleMutPtrType(scope, unwrap.base.src, child_type);
+    var buf: Type.Payload.Pointer = undefined;


It looks like a reference to buf is escaping this function.

I think this would be fixed simply by allocating buf with scope.arena(). Possibly as an optimization, Type.zig could have another function for accepting an allocator that would only allocate buf if it needed to.

The resulting child type is immediately copied but an optionalChildAlloc would be more explicit.

Oh! I see the copy. This actually is a really nice API. I like what you did here 👍

andrewrk · 2020-08-15T18:52:52Z

src-self-hosted/Module.zig

+        return self.constInst(scope, inst.src, .{ .ty = dest_type, .val = val });
+    }
+
+    // TODO how do we get the result location


If you need a pointer to write through, you would emit an alloc instruction. However, for cases where it could be treated as a value (such as here) I think what you have here is better. Since the zig language does not guarantee a memory layout for optionals, that means codegen.zig is allowed to represent the optional value in any way it chooses to. For some combinations of architectures and payload types, this may mean that it uses a single register to represent both the optional bit and the payload. I think it makes sense to make Module.zig / zir_sema.zig not have to care about such details.

andrewrk · 2020-08-15T18:56:39Z

src-self-hosted/codegen.zig

@@ -2004,9 +2023,9 @@ fn Function(comptime arch: std.Target.Cpu.Arch) type {
            return mcv;
        }

-        fn genTypedValue(self: *Self, src: usize, typed_value: TypedValue) !MCValue {
+        fn genTypedValue(self: *Self, src: usize, typed_value: TypedValue) error{ CodegenFail, OutOfMemory }!MCValue {


Suggested change

fn genTypedValue(self: *Self, src: usize, typed_value: TypedValue) error{ CodegenFail, OutOfMemory }!MCValue {

fn genTypedValue(self: *Self, src: usize, typed_value: TypedValue) InnerError!MCValue {

andrewrk · 2020-08-15T19:18:22Z

src-self-hosted/astgen.zig

+            if (cond_kind != .optional) {
+                return mod.failNode(scope, payload, "else payload invalid on bool conditions", .{});
+            }


The presence of a payload capture here would make this an error union if (same deal with while below)

Is that documented anywhere? I think it might be better to require the explicit |_| since that already seems to be more common.

Hmm maybe I'm misunderstanding what the code is doing here. I'm thinking about this:
https://ziglang.org/documentation/master/#while-with-Error-Unions

When the else |x| syntax is present on a while expression, the while condition must have an Error Union Type.

What would be example zig code that would trigger this "else payload invalid on bool conditions" compile error?

if (errUnion) {} else |err| {} // else payload invalid on bool conditions if (errUnion) |_| {} else |err| {} // explicitly ignoring the payload

Looking at the stdlib and tests explicitly ignoring the payload seems more common but that might be just people (like me) not realizing that it isn't necessary.

Ahhh now I see. Hmm. Here's my proposal:

if (errUnion) {} else |err| {} on E!T where T is void should be allowed, but if T is not void then it should be "error: expression value is ignored" same as if you called a non-void function and didn't capture or discard the return value.

Sounds good to me!

andrewrk · 2020-08-15T19:20:30Z

src-self-hosted/astgen.zig

+        const payload = payload_node.?.castTag(.PointerPayload).?;
+        const is_ptr = payload.ptr_token != null;
+        const ident_node = payload.value_symbol.castTag(.Identifier).?;
+        const ident_name = try identifierTokenString(mod, &then_scope.base, ident_node.token);


Elsewhere in stage2 we've treated _ as the "discard" identifier, but @"_" as a normal identifier. Personally I could be persuaded to do it either way, but it should probably be consistent.

Didn't realize this was a thing. Should we try to somehow also apply it to primitive types to fix #1802?

That's a neat idea! Let's do a separate proposal for that, but I think it might be a good solution to the "sometimes we want to use names that collide with primitive types" problem.

andrewrk

I think this is just pending tests passing now

See ziglang#6060 (comment)

Vexu force-pushed the stage2 branch from 3c9dbe9 to 2f14430 Compare August 15, 2020 17:48

andrewrk requested changes Aug 15, 2020

View reviewed changes

Vexu mentioned this pull request Aug 15, 2020

Allow declarations with names that collide with primitive types when using @"" syntax #6062

Closed

Vexu force-pushed the stage2 branch from 2f14430 to a8a5a0e Compare August 16, 2020 18:54

kubkon added the frontend Tokenization, parsing, AstGen, Sema, and Liveness. label Aug 16, 2020

andrewrk approved these changes Aug 17, 2020

View reviewed changes

Vexu added 4 commits August 17, 2020 14:24

stage2: astgen for ptr types and address of

c52513e

stage2: fix optimization causing wrong optional child types

012fac2

stage2: astgen for if and while with optionals

db77b6b

stage2: astgen for if and while with error unions

ece4a2f

Vexu force-pushed the stage2 branch from a8a5a0e to cc1157a Compare August 17, 2020 11:29

address review feedback

13b2f1e

Vexu force-pushed the stage2 branch from cc1157a to 13b2f1e Compare August 17, 2020 16:23

Vexu merged commit d8fb377 into ziglang:master Aug 17, 2020

Vexu mentioned this pull request Apr 1, 2021

Proposal: obligatory captures #8400

Closed

Vexu added a commit to Vexu/zig that referenced this pull request Oct 5, 2022

stage2: add error for non-void error union payload being ignored

ead2203

See ziglang#6060 (comment)

Vexu added a commit to Vexu/zig that referenced this pull request Oct 5, 2022

stage2: add error for non-void error union payload being ignored

f207c74

See ziglang#6060 (comment)

Vexu added a commit to Vexu/zig that referenced this pull request Oct 5, 2022

stage2: add error for non-void error union payload being ignored

6681c18

See ziglang#6060 (comment)

Vexu added a commit to Vexu/zig that referenced this pull request Oct 6, 2022

stage2: add error for non-void error union payload being ignored

0b1dd84

See ziglang#6060 (comment)

TUSF pushed a commit to TUSF/zig that referenced this pull request May 9, 2024

stage2: add error for non-void error union payload being ignored

8566143

See ziglang#6060 (comment)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stage2: more optionals stuff #6060

Stage2: more optionals stuff #6060

Vexu commented Aug 15, 2020

andrewrk left a comment

andrewrk Aug 15, 2020

andrewrk Aug 15, 2020

Vexu Aug 15, 2020

andrewrk Aug 15, 2020

Vexu Aug 15, 2020

andrewrk Aug 15, 2020

andrewrk Aug 15, 2020

andrewrk Aug 15, 2020 •

edited

Loading

Vexu Aug 15, 2020

andrewrk Aug 15, 2020

andrewrk Aug 15, 2020

andrewrk Aug 15, 2020

andrewrk Aug 15, 2020

Vexu Aug 15, 2020

andrewrk Aug 15, 2020

Vexu Aug 15, 2020

andrewrk Aug 15, 2020

Vexu Aug 15, 2020

andrewrk Aug 15, 2020

Vexu Aug 15, 2020

andrewrk Aug 15, 2020

andrewrk left a comment

	fn genTypedValue(self: *Self, src: usize, typed_value: TypedValue) error{ CodegenFail, OutOfMemory }!MCValue {
	fn genTypedValue(self: *Self, src: usize, typed_value: TypedValue) InnerError!MCValue {

Stage2: more optionals stuff #6060

Stage2: more optionals stuff #6060

Conversation

Vexu commented Aug 15, 2020

andrewrk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewrk Aug 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewrk left a comment

Choose a reason for hiding this comment

andrewrk Aug 15, 2020 •

edited

Loading