Skip to content

stage1: support @alignOf(T) aligned member inside struct T #1845

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

Merged
merged 2 commits into from
Dec 20, 2018

Conversation

kristate
Copy link
Contributor

Fixes #1832

  • Implementation
  • Documentation (comments)
  • Tests

@kristate
Copy link
Contributor Author

kristate commented Dec 20, 2018

@bdoner All tests passed; can you please test your code regarding #1832 ?

@svc-user
Copy link

Will check it out later this evening!

@andrewrk
Copy link
Member

Here's a problematic case:

export fn entry() void {
    var n1: Node1 = undefined;
    var n2: Node2 = undefined;
}

const Node1 = struct {
    data: [@alignOf(Node1)]u8,
};

const Node2 = struct {
    data: [1]u8,
};
  %n1 = alloca %Node1, align 8
  %n2 = alloca %Node2, align 1

This will also cause under-alignment problems when #1512 is implemented. The problem is that the assumption that if we hit a recursion loop then the answer is pointer alignment is not always correct.

A more correct solution involves a larger change that introduces "lazy type values" where you can query information, such as, "are you zero bits or not?", "what is your alignment?" while minimally evaluating the sub-expressions in the type. For example, for the expression []align(@alignOf(Node)) Node, we can answer the alignment query without evaluating the expression @alignOf(Node) because the pointer alignment is irrelevant to the query; based on the fact that it is a slice we already know it is pointer size aligned.

That said, this is stage1, and this is an improvement. So I think we can merge this and I'll make a separate issue for the problematic cases.

@andrewrk andrewrk merged commit fb81b19 into ziglang:master Dec 20, 2018
@andrewrk
Copy link
Member

Merged. Thanks @kristate

@kristate
Copy link
Contributor Author

A more correct solution involves a larger change that introduces "lazy type values" where you can query information, such as ...

Yes, I spent a day thinking about such a system and even thought about piggybacking ontop of ResolveStatus and friends, but in the end I decided on this solution as a stop-gap until copy elision lands.

It would be nice to have some sort of graph to query like you mentioned.

@svc-user
Copy link

I tried but failed to build zig on Windows. I trust that you, kristate, and andrewrk are competent enough to test it properly.

Thanks for the swift response and resolution.

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.

3 participants