-
Notifications
You must be signed in to change notification settings - Fork 545
Summarize the lecture of ty into a chapter #530
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
Conversation
cc @nikomatsakis who gave the lecture... |
Who reviews this? Niko? |
e1d19fe
to
7fc7acb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a few random comments from a quick skim through the first little bit
8420959
to
b7db53f
Compare
This is now ready for review. CI failures are unrelated. cc @nikomatsakis |
2fb7d70
to
f2f1325
Compare
I will be fixing the CI failure due to changes of how early lints are defined today, which should hopefully fix CI on this PR. |
@Mark-Simulacrum : @JohnTitor made a PR to fix the links, but we would still really appreciate a PR to update the section on early lints, if you have time :) |
Ping @nikomatsakis :) We would love to get this merged |
where they can be (so, if they do not contain any inference variables or other "temporary" types, | ||
they will be allocated in the global arena). However, the lifetime `'tcx` is always a safe | ||
approximation, so that is what you get back. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be interested to know examples of when an arena would start and stop - is global=crate level?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe global means the whole compilation (ie the whole crate graph)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gilescope thanks for noting that. Can you open an issue that ask for that information?. I think would be better if we get this PR in and we continue with follow ups.
src/ty.md
Outdated
|
||
A bit more deeply, this corresponds to structs in Rust being *nominal* types — which means that they | ||
are defined by their *name* (and that their contents are then indexed from the definition of that | ||
name, and not carried along “within” the type itself). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be possible having set out the above to explain how to query if a ty:Ty implements a specific Trait?
(Thank you so much for this chapter - it is like the rosetta stone)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gilescope Perhaps take a look at the trait-solving chapters (already exist). Does that answer your question?
Ping @spastorino Could you address the review comments above? Also, is the hackmd still the canonical version of the chapter or this PR? Ping @nikomatsakis |
The Hackmd document is the canonical version of the chapter yeah. When some review is made I sync both things. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is awesome! I left a bunch of comments. If you like, I can try to apply some of the edits myself. Let me know. In general, I think we should hurry and land this, and we can do successive edits and improvements as follow-up PRs -- but I did find some important inaccuracies that ought to be addressed.
@spastorino I addressed some of the low-hanging fruit in the hackmd. Also, it looks like the followup PR should create the following (sub)chapters:
|
Just pushed with @mark-i-m fixes and my fixes. |
1364945
to
2031499
Compare
@rust-lang/wg-learning I've resolved the remaining comments and rebased over master. The hackmd and this PR should be consistent. If it looks good, then let's go ahead and merge, and subsequent comments can go in a followup. Both @spastorino and I are going to be unavailable next week, btw... |
3099bdb
to
955bd5f
Compare
This is coming out as part of the effort from the @rust-lang/wg-learning working group 🎉 🎉 🎉.
As currently is I have two comments that may worth fixing:
This may be better fixed using this link, then I can place the fixes into the PR.
Closes #375