Skip to content

Makes the declaration slice resolve lazely when using @typeInfo. #4435

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Makes the declaration slice resolve lazely when using @typeInfo. #4435

wants to merge 1 commit into from

Conversation

FireFox317
Copy link
Contributor

@FireFox317 FireFox317 commented Feb 11, 2020 •
edited
Loading

This fixes issues #3893, #2584 etc.

@FireFox317 FireFox317 marked this pull request as ready for review February 12, 2020 00:38
@FireFox317 FireFox317 changed the title Makes the declaration slice in the @typeInfo lazy. Makes the declaration slice resolve lazely when using @typeInfo. Feb 12, 2020
@FireFox317 FireFox317 force-pushed the lazy-typeinfo-decls branch 3 times, most recently from f23a976 to 31c1f0a Compare February 12, 2020 18:39
@FireFox317
Makes the declaration slice resolve lazely when using @typeInfo
d8961f4 
This way all the declarations in a container won't be resolved untill
the user actually uses the decls slice in the builtin TypeInfo union.
@mikdusan mikdusan added the stage1 The process of building from source via WebAssembly and the C backend. label Feb 12, 2020
@FireFox317
Copy link
Contributor Author

FireFox317 commented Feb 13, 2020 •
edited
Loading

https://github.com/ziglang/zig/pull/4435/files#diff-d03d33b1588c4f305b248019071b9546L1349-L1366

The test above was deleted, because now the correct compile error is given:

./zig-cache/o/.../tmp.zig:2:19: error: expected type 'type', found 'void'
    fn crash() bug() {
                  ^
./zig-cache/o/.../tmp.zig:10:21: note: referenced here
    var boom = start.crash();

Maybe i should have kept it and changed it instead, that is the debatable thing here.

Btw, i guess the windows CI doesn't have its day.
Edit: Now it passed :)

@FireFox317 FireFox317 closed this Feb 13, 2020
@FireFox317 FireFox317 reopened this Feb 13, 2020
This was referenced Feb 16, 2020
Closed
Closed
@andrewrk andrewrk mentioned this pull request Feb 18, 2020
Closed
andrewrk
andrewrk reviewed Feb 18, 2020
View reviewed changes
src/ir.cpp
out_val->special = ConstValSpecialLazy;
out_val->type = get_slice_type(ira->codegen, ptr_type);

LazyValueTypeInfoDecls *lazy_type_info_decls = heap::c_allocator.create<LazyValueTypeInfoDecls>();
Copy link
Member

Choose a reason for hiding this comment

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

This makes 1 lazy value for the entire thing, which means that if you access one decl, it brings in everything. I think this would be better to make a lazy value for each decl.

Or maybe this is still good and it can be a multi-layered lazy thing, and in resolving LazyValueIdTypeInfoDecls it would create lazy values for each element.

I think this is related to #4495.

It's the end of a long 2 days for me, I'm going to come back to this tomorrow, and we'll get this merged 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I did that intentionally, because it seemed easier to implement at that point. However, I can now try to make each element a lazy value, will do that tonight after work :)

One thing I'm wondering is, what should happen when the slice is used in a for loop. Should it then resolve every value in the range of the slice?

andrewrk
andrewrk approved these changes Feb 18, 2020
View reviewed changes
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.

I had another look today, and I think this is good to merge as-is. Nice work!

I'm merging locally in a branch to add a behavior test case.

@andrewrk andrewrk closed this in 662996e Feb 18, 2020
@ghost ghost mentioned this pull request Feb 26, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andrewrk andrewrk andrewrk approved these changes

Assignees
No one assigned
Labels
stage1 The process of building from source via WebAssembly and the C backend.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@FireFox317 @andrewrk @mikdusan