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

tutorial: add classes #2838

Merged
merged 1 commit into from
Jul 8, 2012
Merged

Conversation

gwillen
Copy link
Contributor

@gwillen gwillen commented Jul 8, 2012

Adds a section on classes to the tutorial. Criticism of course welcomed. It might be too verbose. Happy for suggested changes to be made either before or after merging. :-)

@catamorphism
Copy link
Contributor

I'm not sure we usually include bug numbers in the tutorial -- the tutorial is meant to be based on how the language should be, and the bug is likely to be fixed by the time someone reads the tutorial. Do you mind re-doing it without those? Thanks!

@gwillen
Copy link
Contributor Author

gwillen commented Jul 8, 2012

heh, yes, bblum suggested to me in his review that this was probably the wrong thing.

I would prefer to document the compiler as it currently exists, though, and not as it will one day be, for semi-obvious reasons.

So I guess the best thing to do is 1) not mention static methods, since they don't exist; 2) not mention the constructor and destructor bug, since it will likely be fixed and is nonessential to know about in any case; 3) document the same-instance restriction, as presently implemented? I definitely don't want to document it as-may-someday-be-implemented, since that would be misleading.

Is there a way to update an existing pull request, or do I have to drop and refile it?

@catamorphism
Copy link
Contributor

Re; 1 and 2, yes, I agree. Re: 3, I would say that this restriction exists but that it may change in the future, without referring to the bug number. Thanks again, in general it looks good!

I think if you just do a new pull request, github will update the right issue... but it's been so long since I did one myself, I'm not totally sure...

@gwillen
Copy link
Contributor Author

gwillen commented Jul 8, 2012

Oh, sorry, one more question: Is it safe for me to squash the commits together once I make the additional changes, and before resubmitting the pull request, even though the first commit has been seen by the outside world? It seems like it should be, since it hasn't been pushed to mozilla/rust, only gwillen/rust, which nobody else should care about.

@catamorphism
Copy link
Contributor

It's ok to squash, because this hasn't been merged into incoming yet.

@gwillen
Copy link
Contributor Author

gwillen commented Jul 8, 2012

Ok, try now. (Apparently pull requests are by default tied to a branch, not a commit, so I can just push to the branch (or push -f in this case, to squash history down to one commit) and it updates automatically, creepily rewriting the history of the pull request.)

catamorphism added a commit that referenced this pull request Jul 8, 2012
@catamorphism catamorphism merged commit 9284829 into rust-lang:incoming Jul 8, 2012
@catamorphism
Copy link
Contributor

Merged, thanks!

@gwillen
Copy link
Contributor Author

gwillen commented Jul 8, 2012

Sweeet! Thanks for your help and feedback. :-)

RalfJung pushed a commit to RalfJung/rust that referenced this pull request Apr 28, 2023
throw unsupported for `epoll_wait`

This PR throws unsupported to indicate miri doesn't yet return ready events. Previously it always returned 0, indicating no ready events, even if events for the epoll file descriptor may have been ready.
celinval pushed a commit to celinval/rust-dev that referenced this pull request Jun 4, 2024
Update Rust toolchain from nightly-2023-10-19 to nightly-2023-10-20
without any other source changes.
This is an automatically generated pull request. If any of the CI checks
fail, manual intervention is required. In such a case, review the
changes at https://github.com/rust-lang/rust from
rust-lang@0039d73
up to
rust-lang@4578435.
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.

2 participants