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

Remove subscription_t's clock.identifier field. #125

Merged

Conversation

sunfishcode
Copy link
Member

As noticed in
rust-lang/rust#65617 (comment),
the clock.identifier field in subscription_t isn't used for
anything. It came from CloudABI, where it appears to be a holdover from
an earlier API feature which is no longer present.

As noticed in
rust-lang/rust#65617 (comment),
the `clock.identifier` field in `subscription_t` isn't used for
anything. It came from CloudABI, where it appears to be a holdover from
an earlier API feature which is no longer present.
@@ -552,8 +552,6 @@
;; The contents of a $subscription_t when type is `EVENTTYPE_CLOCK`.
(typename $subscription_clock_t
(struct
;; The user-defined unique identifier of the clock.
(field $identifier $userdata_t)
Copy link

Choose a reason for hiding this comment

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

This may theoretically be useful for debuggers.

Copy link
Member Author

Choose a reason for hiding this comment

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

What would it add beyond the clock_id field?

Copy link

@bjorn3 bjorn3 Oct 22, 2019

Choose a reason for hiding this comment

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

I though I read that it was a string. In that case, a debugger could show that string to the user. I think it would be better to design such debugger integrations separately and for all things it makes sense for at a later time though.

@dunnock
Copy link

dunnock commented Oct 24, 2019

BTW, what is the right way to land this spec change in the code, e.g. is it ok to just remove it from the structure in https://github.com/CraneStation/rust-wasi and fix dependencies in other projects? E.g.
https://github.com/kubkon/wasi-misc-tests/commit/4801d40c895045bfdb36a17d60e9fd36d8ace798
https://github.com/rust-lang/rust/blob/master/src/libstd/sys/wasi/thread.rs#L37

If so it seems quite simple, I can take it.

@newpavlov
Copy link

@dunnock
Since it's a breaking change, I think it's worth to synchronize it with other planned Core API changes and release them in a single update.

@dunnock
Copy link

dunnock commented Oct 25, 2019

@newpavlov thanks, it does not seem an entry level stuff then

@sunfishcode
Copy link
Member Author

Yes, following the phases document, this change is happening to ephemeral, which we'll batch up with other changes and snapshot to a versioned interface. As for keeping rust-wasi up to date, I'm working on a way to make that easier; hopefully I'll have the PR ready soon.

@sunfishcode sunfishcode merged commit f5d6ef0 into WebAssembly:master Oct 31, 2019
@sunfishcode sunfishcode deleted the subscription.clock.identifier branch October 31, 2019 20:50
cjihrig added a commit to cjihrig/wasi-libc that referenced this pull request Nov 15, 2019
cjihrig added a commit to nodejs/uvwasi that referenced this pull request Dec 4, 2019
cjihrig added a commit to nodejs/uvwasi that referenced this pull request Dec 14, 2019
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.

6 participants