Uh oh!
There was an error while loading. Please reload this page.
Move str to libcore. - #70911
Closed
eddyb wants to merge 3 commits into
Closed
Conversation
eddyb
commented
Apr 8, 2020
ContributorAuthor
@bors try @rust-timer queue |
rust-timer
commented
Apr 8, 2020
Collaborator
Awaiting bors try build completion |
bors
commented
Apr 8, 2020
Collaborator
⌛ Trying commit 137c171 with merge f95a789e60e4d3fbb4e0c82de224af9d890e5b75... |
rust-highfive
commented
Apr 8, 2020
Contributor
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
bors
commented
Apr 8, 2020
Collaborator
☀️ Try build successful - checks-azure |
rust-timer
commented
Apr 8, 2020
Collaborator
Queued f95a789e60e4d3fbb4e0c82de224af9d890e5b75 with parent 42abbd8, future comparison URL. |
SimonSapin
commented
Apr 8, 2020
Contributor
Simplifying the language by pushing more of it into the standard library sounds nice from a theoretical point of view, but what is the practical benefit that we would gain by having someone spend the bandwidth to finish this? |
RalfJung
reviewed
Apr 8, 2020
| ) -> InterpResult<'tcx> { | ||
| match op.layout.ty.kind { | ||
| ty::Str => { | ||
| // FIXME(eddyb) does this belong here? |
Member
There was a problem hiding this comment.
Well you could remove that branch and then str would not be special any more in terms of validity. ;)
rust-timer
commented
Apr 8, 2020
Collaborator
Finished benchmarking try commit f95a789e60e4d3fbb4e0c82de224af9d890e5b75, comparison URL. |
eddyb
commented
Apr 8, 2020
ContributorAuthor
Not sure there is much to finish, it's more about keeping this working and adjusting it due to review comments, throughout the approval process, whichever teams would decide on it. But also, wow, this is a slowdown. It really shouldn't, I think every single place in the compiler this PR makes slower also has been slowing down user-defined types w/o parameters this entire time, so it might be worth digging into some of these and trying to fix them. |
Contributor
The previous attempt: #19612. I wanted to get this done in 2014 and I still want to get it done. |
oli-obk
commented
Apr 8, 2020
Contributor
|
oli-obk
reviewed
Apr 8, 2020
| Ok(Some(self.read_size_and_align_from_vtable(vtable)?)) | ||
| } | ||
| ty::Slice(_) | ty::Str => { |
Contributor
There was a problem hiding this comment.
potential site for the perf regression, easy to test by doing the old logic if ty.is_str()
| pub fn read_str(&self, mplace: MPlaceTy<'tcx, M::PointerTag>) -> InterpResult<'tcx, &str> { | ||
| let len = mplace.len(self)?; | ||
| let bytes = self.memory.read_bytes(mplace.ptr, Size::from_bytes(len))?; | ||
| let bytes_mplace = self.mplace_field(mplace, 0)?; |
Contributor
There was a problem hiding this comment.
potential site for the perf regression. We should probably check whether we can make mplace_field a no-op in case of repr(Rust) with one field or repr(transparent) types.
I think the main problem here is that we compute the field's layout even for cases like this one where we don't actually need the layout. We've seen major regressions in miri due to a single additional call to tcx.layout_of before.
petrochenkov
commented
Apr 8, 2020
Contributor
Note: the diff is actually negative if you ignore the newly added tests. |
petrochenkov
commented
Apr 8, 2020
Contributor
Reviewed. It feels like most of the special treatment in diagnostics can be removed if we officially recognize that The special case in mangling can be removed as well, except that it would needs a version bump. |
petrochenkov
commented
Apr 8, 2020
Contributor
👍 |
eddyb
commented
Apr 8, 2020
ContributorAuthor
I haven't done any testing on this but I wonder if:
We sort of had plans for that, along the lines of representing paths like However, I realize now that we could encode it as We shouldn't need a bump to make the |
eddyb
commented
Apr 8, 2020
ContributorAuthor
To expand on this a bit: one of the unshadowed_prelude_items:FxHashMap<DefId,Symbol>,that we use in |
Contributor
Yes,
It would, but that's not a big deal? EDIT: Unless you were talking about error messages here, |
eddyb
commented
Apr 9, 2020
ContributorAuthor
Neat! Presumably scanning for locally defined items with the same names would be more effort, but maybe there's some map (oh, there's the
Sorry, yeah, my concerns are:
|
Contributor
Thoughts from the language team meeting yesterday:
So in conclusion, I think we can green-light this from a lang team POV (regressions in perf etc. are for the compiler team to assess, and fix), but let's also be careful not to lock us into a world where this cannot be reverted. |
petrochenkov
commented
May 19, 2020
Contributor
Closing due to inactivity. |
ContributorAuthor
It's possible #73996 unblocked the "diagnostics should still say |
27 tasks
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
9 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I was reminded we tried to do this before and didn't go through with it for some reason, after seeing rust-lang/unsafe-code-guidelines#78 (the impact on this PR on that question isn't significant, FWIW).
One thing I didn't do is change how
stris resolved: it's still a builtin type forrustc_resolve, which only after name resolution is turned into the right type, using the"str"lang item.(Presumably if
core::strhadcrate mod sealed { pub struct Str([u8]); }thencore::preludecould dopub use crate::str::sealed::Str as str;, but I haven't tried it - IMO a requirement is that users can't observecore::str::strorcore::str::Str. cc @petrochenkov)While this does seem to work with enough tweaks, I don't have the bandwidth to push this through.
So I suppose this could be considered insta-abandonware ♻️.
r? @ghost cc @rust-lang/libs @rust-lang/compiler