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

WIP Interface Reform (see: #1829) #1848

Closed
wants to merge 15 commits into from
Closed

WIP Interface Reform (see: #1829) #1848

wants to merge 15 commits into from

Conversation

tgschultz
Copy link
Contributor

@tgschultz tgschultz commented Dec 20, 2018

Update the standard library to use a new interface pattern that should be more friendly to the optimizer. Provides abstracted (type-erased) and non-abstracted interfaces.

Note: the way async<allocator> is currently implemented appears to be incompatible with the this pattern. Since the async rewrite (#1260) will eliminate allocators for async, I have implemented an ugly hack workaround that wraps the new interface pattern with the old one.

Current plan:

  • Update mem.Allocator interface
    • Update test cases, self hosted, other non-std code
  • Update io.(In|Out)Stream interfaces
    • Update test cases, self hosted, other non-std code

@tiehuis will take care of these
- [ ] Update Random interface
- [ ] Update test cases, self hosted, other non-std code

This may end up being a separate PR:

  • Update all stdlib uses of interfaces to allow for use of non-abstracted version

We may also decide to move the 'convenience' functions currently implemented in the generated *Interface struct out into the broader namespace as regular functions.

@tiehuis
Copy link
Member

tiehuis commented Dec 20, 2018

I've got a local copy of the randomizer changes nearly done so you can leave that to me if you like.

@tgschultz
Copy link
Contributor Author

Currently this is stalled by the CI failing and my lack of a complete enough build environment to reproduce and diagnose the issue (which probably has to do with async). That may be resolved soon, but worst case this is blocked until after async is reworked (which is after copy elision).

@andrewrk
Copy link
Member

andrewrk commented Jan 2, 2019

It'll happen. Thanks for being patient with my copy elision stuff. I'm at 327/518 behavioral tests passing in my branch. I'm hoping to conclude the merge within the next week.

@tgschultz
Copy link
Contributor Author

tgschultz commented Jan 7, 2019

It was async. I'd missed some spots because I wasn't grepping properly. I've built a new VM server and now have a much less constrained environment, so I can run tests locally and work through stuff like this.

@tgschultz
Copy link
Contributor Author

Unless he's changed his mind, I'm going to leave Random to @tiehuis .

@tiehuis
Copy link
Member

tiehuis commented Jan 18, 2019

Yeah I have it done although I've had some issues with attempting to get it to work at compile-time as the old method was able to. Will make a PR when this is merged and I can rebase off this.

@Hejsil
Copy link
Contributor

Hejsil commented Jan 18, 2019

@tiehuis Is this related? The vtable dynamic dispatch doesn't work at comptime, but any static dispatch should work just fine right?

@tiehuis
Copy link
Member

tiehuis commented Jan 18, 2019

Yes, that is exactly the issue. Didn't see that. It isn't a huge deal right now, the only place that was depending on this was AutoHashMap which I'm sure we can perform at runtime until this is figured out.

@Hejsil
Copy link
Contributor

Hejsil commented Jan 18, 2019

@tiehuis Could the signature for autoHashs rng parameter be of type var and then we pass a statically known Random implementer? This should work at comptime

@tgschultz tgschultz closed this Jan 20, 2019
@tgschultz tgschultz reopened this Jan 20, 2019
@andrewrk andrewrk added the work in progress This pull request is not ready for review yet. label Feb 7, 2019
@andrewrk
Copy link
Member

What should we do with this PR? What can I do to help?

@tgschultz
Copy link
Contributor Author

My plan was to wait on this until after the co-routing rewrite, as currently a pretty ugly hack is used to deal with async's allocator handling. If we want to merge before then and are ok with the hack (or have a better idea) then I can rebase on current master, clean it up, add @tiehuis's changes and it should be ready to go.

@andrewrk
Copy link
Member

Waiting until after the coroutine rewrite sounds reasonable. I'll keep that in mind. It's planned to happen like this:

  • 0.4.0 released on April 8th (estimated date; final date depends on llvm 8.0.0 official release)
  • Blog post with detailed Zig Package Manager proposal. I do reserve the right to re-arrange this to be the last bullet point in this list, since it depends on the following 2 items.
  • Non-copy semantics part 1a.
  • Coroutine rewrite.

These last 2 bullet points are long overdue. I'm planning on giving them laser focus after the release.

@andrewrk
Copy link
Member

I made a note in #2377 (comment) to revisit this work-in-progress code once the stuff that it is blocking on is complete. Until then I'm going to close this.

@andrewrk andrewrk closed this Apr 29, 2019
@tgschultz tgschultz deleted the stdlib-interface_reform branch May 22, 2019 22:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
work in progress This pull request is not ready for review yet.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants