Skip to content

Fix rustdoc #43691

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Aug 9, 2017
Merged

Fix rustdoc #43691

merged 3 commits into from
Aug 9, 2017

Conversation

GuillaumeGomez
Copy link
Member

Fixes #43625.

r? @rust-lang/dev-tools

cc @rust-lang/docs

QuietMisdreavus
QuietMisdreavus reviewed Aug 5, 2017
View reviewed changes
src/librustdoc/html/markdown.rs Outdated
}
if !added {
full.extend_from_slice(&[b'\\', b'0']);
}
Copy link
Member

Choose a reason for hiding this comment

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

This loop feels suspect. After a long enough read it seems okay, but maybe replacing it with a loop over indices and something like full.splice(idx..(idx+1), b"\\0".iter().cloned()) would make it easier to read its intent.

Copy link
Member

Choose a reason for hiding this comment

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

For what it's worth s.replace("\0", "\\0") would be even easier to read.

@ollie27
Copy link
Member

ollie27 commented Aug 6, 2017

This isn't how I would fix this. I don't think there is any need to escape null bytes as they are valid UTF-8. The issue is with the unwrap on CString::new but we don't need to use CStrings at all here if we use hoedown_buffer_put instead of hoedown_buffer_puts.

Specifically replacing all 3 instances of:

let element = CString::new(content).unwrap();
unsafe { hoedown_buffer_puts(ob, element.as_ptr()); }

with:

unsafe { hoedown_buffer_put(ob, content.as_ptr(), content.len()); }

@estebank
Copy link
Contributor

estebank commented Aug 6, 2017

@GuillaumeGomez the only reason I wouldn't fix this issue this way is because there's an unexpected side effect: now writing \0 and \\0 will result in the same output, and it's only on this escape sequence that this happens. Although the only conceptual problem I can see this causing is making people think that they can't use escape sequences at all, which would be a rather small problem admitedly, and I appreciate the effort to be helpful and do what the user meant instead of what wrote, I feel that a lint pointing at the error would be preferable as it is a teachable moment opportunity to make the user's mental model more accurate. Of course, even with a lint or warning this fix could still apply, if nothing else to allow the compiler to continue.

@GuillaumeGomez
Copy link
Member Author

The problem only exists because of hoedown. This is just a quick fix until we get rid of hoedown definitely (some day...).

@GuillaumeGomez
Copy link
Member Author

Updated, and keep in mind that this "fix" only concerns hoedown and won't be an issue with pulldown.

ollie27
ollie27 reviewed Aug 6, 2017
View reviewed changes
src/librustdoc/html/markdown.rs Outdated
@@ -531,6 +530,7 @@ extern {
fn hoedown_buffer_new(unit: libc::size_t) -> *mut hoedown_buffer;
fn hoedown_buffer_puts(b: *mut hoedown_buffer, c: *const libc::c_char);
fn hoedown_buffer_free(b: *mut hoedown_buffer);
fn hoedown_buffer_put(b: *mut hoedown_buffer, c: *const libc::c_char, len: libc::size_t);
Copy link
Member

Choose a reason for hiding this comment

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

The second parameter is const uint8_t *data so here it should be *const u8. That also means as *const libc::c_char can be removed from below.

src/librustdoc/html/markdown.rs Outdated
let content = format!("<code>{}</code>", Escape(&content));
let element = CString::new(content).unwrap();
unsafe { hoedown_buffer_puts(ob, element.as_ptr()); }
let content = format!("<code>{}</code>", Escape(&content)).replace("\0", "\\0");
Copy link
Member

Choose a reason for hiding this comment

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

This change seems unnecessary. As hoedown just passes null bytes through unmodified everywhere else there is no reason to escape it in this one case.

Copy link
Member Author

Choose a reason for hiding this comment

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

If we don't do it, it doesn't appear in the generated HTML.

Copy link
Member

Choose a reason for hiding this comment

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

It will, but browsers may not show anything but that's expected and what already happens if null bytes appear in normal paragraphs.

Copy link
Member Author

Choose a reason for hiding this comment

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

So be it.

@GuillaumeGomez
Copy link
Member Author

Updated. No more \0 displayed.

@nrc
Copy link
Member

nrc commented Aug 7, 2017

r? @steveklabnik

@arielb1 arielb1 added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 8, 2017
steveklabnik
steveklabnik reviewed Aug 8, 2017
View reviewed changes
Copy link
Member

@steveklabnik steveklabnik left a comment

Choose a reason for hiding this comment

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

This PR seems fine, but I'm still not super mega confident with rustdoc's codebase

src/librustdoc/html/markdown.rs Outdated
@@ -531,6 +530,7 @@ extern {
fn hoedown_buffer_new(unit: libc::size_t) -> *mut hoedown_buffer;
fn hoedown_buffer_puts(b: *mut hoedown_buffer, c: *const libc::c_char);
fn hoedown_buffer_free(b: *mut hoedown_buffer);
fn hoedown_buffer_put(b: *mut hoedown_buffer, c: *const u8, len: libc::size_t);
Copy link
Member

Choose a reason for hiding this comment

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

is hoedown_buffer_puts used elsewhere? This PR seems mostly to be replacing puts with put. If puts isn't, then we should also remove it

Copy link
Member Author

Choose a reason for hiding this comment

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

No, it's still used.

@steveklabnik
Copy link
Member

r=me if @QuietMisdreavus likes it too

@QuietMisdreavus
Copy link
Member

r=me pending travis on this last push

@QuietMisdreavus
Copy link
Member

@bors r+

@bors
Copy link
Collaborator

bors commented Aug 8, 2017

📌 Commit ec0ca3a has been approved by QuietMisdreavus

@bors
Copy link
Collaborator

bors commented Aug 8, 2017

⌛ Testing commit ec0ca3a with merge 78efb23...

bors added a commit that referenced this pull request Aug 8, 2017
@bors
Auto merge of #43691 - GuillaumeGomez:fix-rustdoc, r=QuietMisdreavus
78efb23 
Fix rustdoc

Fixes #43625.

r? @rust-lang/dev-tools

cc @rust-lang/docs
@bors
Copy link
Collaborator

bors commented Aug 9, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: QuietMisdreavus
Pushing 78efb23 to master...

@bors bors merged commit ec0ca3a into rust-lang:master Aug 9, 2017
@GuillaumeGomez GuillaumeGomez deleted the fix-rustdoc branch August 9, 2017 09:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@steveklabnik steveklabnik steveklabnik left review comments

@QuietMisdreavus QuietMisdreavus QuietMisdreavus left review comments

@ollie27 ollie27 ollie27 left review comments

Assignees

@steveklabnik steveklabnik

Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@GuillaumeGomez @ollie27 @estebank @nrc @steveklabnik @QuietMisdreavus @bors @arielb1