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

Solve the return type ambiguity #1628

Merged
merged 22 commits into from
Oct 15, 2018
Merged

Solve the return type ambiguity #1628

merged 22 commits into from
Oct 15, 2018

Conversation

Hejsil
Copy link
Contributor

@Hejsil Hejsil commented Oct 3, 2018

Fixes #760

I chose using . before { as the way to solve this.

I've only made the changes nessesary for stage1 to compile this code:

const A = error.{B};
const S = struct.{a: u8};
const E = enum.{A};
const U1 = union.{A:u8};
const U2 = union(enum).{A:u8};
const U3 = union(E).{A:u8};

export fn a() void {
    var q: A = A.B;
    var s: S = S.{.a = 0};
    var e: E = E.A;
    var uu1: U1 = U1.{.A=0};
    var uu2: U2 = U2.{.A=0};
    var uu3: U3 = U3.{.A=0};
}


pub fn panic(msg: []const u8, error_return_trace: ?*@import("builtin").StackTrace) noreturn {
    unreachable;
}

Next step is to have a zig fmt branch that can translate old code into the new syntax, and then use that to finish this PR.

* <container> { ... } -> <container> . { ... }
* <exrp> { ... } -> <expr> . { ...}
@Hejsil Hejsil added the work in progress This pull request is not ready for review yet. label Oct 3, 2018
Copy link
Member

@andrewrk andrewrk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Let's try to get all the changes in a branch, so that master always has all tests passing, with stage1 and stage2 agreeing on syntax.

Edit: oh, I see, this is the branch, and you're still working on it. 👍

@Hejsil
Copy link
Contributor Author

Hejsil commented Oct 4, 2018

I have all tests passing locally. The solve-return-type-problem-fmt branch can be used to upgrade old code to the new syntax.

@winksaville
Copy link
Contributor

In using . before { it mentions "Remove the requirement of () for if, while, for, switch, etc.". I pulled in the branch and gave it a try on an if statement and it seems they are still required:

$ cat if.zig
const std = @import("std");
const warn = std.debug.warn;

test "if" {
    var i: usize = 1;
    var j: usize = 2;
    var k: usize = undefined;

    //if (i < j) {
    if i < j {
        k = @intCast(usize, 3);
    } else {
        k = @intCast(usize, 4);
    }
    warn("i={} j={} k={}\n", i, j, k);
}

$ ./build/bin/zig test if.zig 
/home/wink/prgs/ziglang/zig/if.zig:10:8: error: expected token '(', found 'Symbol'
    if i < j {
       ^

Is it the long term plan to remove the requirment for the parens?

@Hejsil
Copy link
Contributor Author

Hejsil commented Oct 4, 2018

@winksaville I did not remove this requirement, as the purpose of this PR is to solve the return type ambiguity. Someone else could then remove the requirement in their own PR :)

Copy link
Member

@andrewrk andrewrk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is pending self hosted compiler parsing update

@andrewrk
Copy link
Member

andrewrk commented Oct 8, 2018

@Hejsil when you get back from your trip, I have an idea how we can avoid too much conflicts, when merging this:

  1. make a new branch from master
  2. run the zig fmt fixer upper in the new branch
  3. merge the new branch into this PR branch

hopefully that reduces the number of conflicts.

@Hejsil Hejsil removed the work in progress This pull request is not ready for review yet. label Oct 15, 2018
@Hejsil
Copy link
Contributor Author

Hejsil commented Oct 15, 2018

Jobs done!

@Hejsil Hejsil merged commit 378d3e4 into master Oct 15, 2018
@Hejsil Hejsil deleted the solve-return-type-problem branch October 15, 2018 13:51
@allochi
Copy link

allochi commented Oct 15, 2018

I'm really sorry, this is not in anyway trying to be picky or undermind the great work that you guys are doing, but I don't like this change to the language, I may get used to it, but I have a point.

. has been engraved in programmers mind as a member access operators, just like * for pointers, having it used this way is harmful for readability in my opinion, I checked the new standard library for example, and it doesn't look good, too many confusing dots to define a struct or initialize an instance.

const point = Point.{ .x = 10, .y = 20 };

I respect your decision and I regret not contributing to the original discussion, but I would like to ask to reconsider using . for that purpose.

I wouldn't mind using keywords like define and new which declare their purpose than overloading .

const Point = struct.define {
    x: i32,
    y: i32,
};

const point = Point.new {
    .x = 10,
    .y = 20,
};

and to be quite honest I much prefer the original way without a dot or a keyword.

@Hejsil
Copy link
Contributor Author

Hejsil commented Oct 15, 2018

@allochi It's probably better to post this on the issue instead of here, so we can keep the discussion in one place.

@allochi
Copy link

allochi commented Oct 15, 2018

@Hejsil Thanks, I will do so.

@allochi allochi mentioned this pull request Oct 15, 2018
Hejsil added a commit that referenced this pull request Oct 27, 2018
Hejsil added a commit that referenced this pull request Nov 13, 2018
Reverted #1628 and changed the grammar+parser of the language to not allow certain expr where types are expected
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.

4 participants