-
Notifications
You must be signed in to change notification settings - Fork 13k
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
address some FIXME whose associated issues were marked as closed #56962
Conversation
r? @pnkfelix (rust_highfive has picked a reviewer for you, use r? to override) |
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 |
…t platform specific code remains
…partial initialization
…x hr_lifetime_in_assoc_type warning
This reverts commit d88d319.
8278f7a
to
957a9c7
Compare
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 |
5184788
to
7894717
Compare
@@ -58,7 +58,7 @@ const EXCEPTION_PATHS: &[&str] = &[ | |||
"src/rtstartup", // Not sure what to do about this. magic stuff for mingw | |||
|
|||
// temporary exceptions | |||
"src/libstd/lib.rs", // FIXME(#44217) |
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'd be curious to know if the original intention was that "src/libstd/lib.rs" would eventuallyi not be on this list, but #44217 was blocking that. In which case, what is the intention now...?
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.
right now tidy complains about sgx specific cfg_attr
in here.
This seems fine. I think its going to collide with PR #55937. But we'll see which one lands first. |
(hit the wrong button!) |
@bors r+ |
📌 Commit 7894717 has been approved by |
@bors r- rollup- The commit cc568e7 failed in #57063 (comment). This has forgotten to update the line numbers in |
updated the file based on the changes to |
@nivkner You need to set |
a1c4d8f
to
e32c63e
Compare
@@ -1348,7 +1346,7 @@ impl<'a> SourceCollector<'a> { | |||
cur.push(component); | |||
fs::create_dir_all(&cur).unwrap(); | |||
root_path.push_str("../"); | |||
href.push_str(component); | |||
href.push_str(&component.to_string_lossy()); |
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.
Why not .to_str().unwrap()
(as before in clean_srcpath
)? It will only be a dead link if replacement characters are inserted.
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.
because I saw the same thing being done a few lines below so I assumed that was expected outcome.
also the intent of the FIXME was not failing when non-utf8 paths are encountered (so panicking here would have the same result), I am not sure it necessarily means the resulting link has to work.
@bors r=pnkfelix |
📌 Commit e32c63e has been approved by |
⌛ Testing commit e32c63e with merge 400df515ff224480dce1eb82047267cf90b35bda... |
💔 Test failed - status-travis |
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 retry |
address some FIXME whose associated issues were marked as closed part of #44366
☀️ Test successful - status-appveyor, status-travis |
part of #44366