Skip to content

Fix rustdoc formatting of impls #27103

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

Merged
merged 3 commits into from
Jul 21, 2015
Merged

Fix rustdoc formatting of impls #27103

merged 3 commits into from
Jul 21, 2015

Conversation

wthrowe
Copy link
Contributor

@wthrowe wthrowe commented Jul 18, 2015

This fixes a couple of bugs visible on https://doc.rust-lang.org/nightly/std/marker/trait.Sync.html . For example:

  • impl<T> Sync for *const T should read impl<T> !Sync for *const T
  • impl<T> !Sync for Weak<T> should read impl<T> !Sync for Weak<T> where T: ?Sized

This does change a struct in librustdoc and it seems that almost everything there is marked public, so if librustdoc has stability guarantees that could be a problem. If it is, I'll find a way to rework the change to avoid modifying public structures.

Some cases displayed negative impls as positive, and some were missing
where clauses.  This factors all the impl formatting into one
function so the different cases can't get out of sync again.
@rust-highfive
Copy link
Contributor

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @cmr (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. The way Github handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@steveklabnik
Copy link
Member

librustdoc has no stability guarantees, so that's fine. code looks good to me, but I'd like someone else to sign off on it too.

@tomjakubowski
Copy link
Contributor

Could you add tests for this? The rustdoc test suite isn't terribly well-documented, but see https://github.com/rust-lang/rust/tree/master/src/test/rustdoc for some examples. The script that runs the rustdoc tests is called htmldocck.py. Please ask questions if you have any!

@wthrowe
Copy link
Contributor Author

wthrowe commented Jul 20, 2015

I've added a test covering two of the three cases where impls are currently printed. I think the third is only used for cross-crate implementations of traits, so I'm not sure if there's an easy way to test that. (They all use the same formatting code now, so I don't think it's all that important to test the last case.)

@tomjakubowski
Copy link
Contributor

Thanks so much! Yeah, cross-crate rustdoc tests are a pain (and unfortunately, also the kind of docs that break the most often). What you want to do for those is write a crate file in src/test/auxiliary, and then import that crate from a crate in src/test/rustdoc (and pub use whatever you need to in order to exercise the test).

Here's an example:

rustdoc test crate: https://github.com/rust-lang/rust/blob/master/src/test/rustdoc/default-impl.rs
auxiliary test crate: https://github.com/rust-lang/rust/blob/master/src/test/auxiliary/rustdoc-default-impl.rs

@alexcrichton
Copy link
Member

Looks great to me as well, thanks @wthrowe! r=me if a test like @tomjakubowski mentioned is added.

@wthrowe
Copy link
Contributor Author

wthrowe commented Jul 20, 2015

Thanks for the tips! Here's another test!

@alexcrichton
Copy link
Member

@bors: r+ dd82df6

Thanks!

bors added a commit that referenced this pull request Jul 20, 2015
This fixes a couple of bugs visible on https://doc.rust-lang.org/nightly/std/marker/trait.Sync.html .  For example:
* `impl<T> Sync for *const T` should read `impl<T> !Sync for *const T`
* `impl<T> !Sync for Weak<T>` should read `impl<T> !Sync for Weak<T> where T: ?Sized`

This does change a struct in librustdoc and it seems that almost everything there is marked public, so if librustdoc has stability guarantees that could be a problem.  If it is, I'll find a way to rework the change to avoid modifying public structures.
@bors
Copy link
Collaborator

bors commented Jul 20, 2015

⌛ Testing commit dd82df6 with merge 676bec3...

@bors
Copy link
Collaborator

bors commented Jul 21, 2015

💔 Test failed - auto-linux-64-x-android-t

@alexcrichton
Copy link
Member

Ah right I believe that all cross-crate rustdoc tests need some special sauce for android, I believe you can fix this by just adding // ignore-cross-compile at the top.

@wthrowe
Copy link
Contributor Author

wthrowe commented Jul 21, 2015

See if that does it.

@alexcrichton
Copy link
Member

@bors: r+ a3e78f4

bors added a commit that referenced this pull request Jul 21, 2015
This fixes a couple of bugs visible on https://doc.rust-lang.org/nightly/std/marker/trait.Sync.html .  For example:
* `impl<T> Sync for *const T` should read `impl<T> !Sync for *const T`
* `impl<T> !Sync for Weak<T>` should read `impl<T> !Sync for Weak<T> where T: ?Sized`

This does change a struct in librustdoc and it seems that almost everything there is marked public, so if librustdoc has stability guarantees that could be a problem.  If it is, I'll find a way to rework the change to avoid modifying public structures.
@bors
Copy link
Collaborator

bors commented Jul 21, 2015

⌛ Testing commit a3e78f4 with merge 238765e...

@bors bors merged commit a3e78f4 into rust-lang:master Jul 21, 2015
@wthrowe wthrowe deleted the doc_format branch August 10, 2015 04:18
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.

7 participants