Skip to content
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

Rustdoc: memoize pub use-reexported macros so they don't appear twice in docs #40814

Merged
merged 2 commits into from
Mar 30, 2017
Merged

Rustdoc: memoize pub use-reexported macros so they don't appear twice in docs #40814

merged 2 commits into from
Mar 30, 2017

Conversation

abonander
Copy link
Contributor

Closes #39436

Preserves existing behavior for #[macro_reexport]. pub use'd macros are shown as reexports unless inlined, and also correctly obey #[doc(hidden)].

r? @jseyfried

cc @SergioBenitez

@abonander
Copy link
Contributor Author

I don't have a test for this because I don't have a clue where to start writing one. run-make + grep again?

@jseyfried
Copy link
Contributor

This looks correct but seems like a step in the wrong direction w.r.t. complexity.

Could we get instead get rid of this block entirely so that macro re-exports are not inlined by default and then push to om.macros here when we do inline instead of removing from export_map when we don't inline?

I believe that would be equivalent, except that #[macro_reexported] macros would no longer get documented. To address this, we could

  • deprecate #![feature(macro_reexport)] in favor of #![feature(use_extern_macro)] (I've been meaning to do this for awhile), and/or
  • keep a Vec of #[macro_reexported]ed macro in the Resolver and push to these to om.macros directly in rustdoc.

@abonander
Copy link
Contributor Author

The second option sounds like it'll require more complexity and touch more code, and I was hesitant to break documentation for #[macro_reexport], whether we want to deprecate it or not.

Perhaps we can fix the issue first, then deprecate #[macro_reexport] and have a patch for the first option ready to merge alongside the attribute's removal? Going from this fix to the first option basically just involves moving some code around and reverting the need for RefCell on export_map.

@abonander
Copy link
Contributor Author

"the first option" being your suggested changes; I just don't like the idea of breaking documentation for #[macro_reexport] even if it becomes deprecated--it's not really fair to the user, IMO.

@jseyfried
Copy link
Contributor

Ok, makes sense. I still prefer the second option, but this is fine for now, especially since we can switch to the first option when removing #[macro_reexport] as you suggested.

I think you can write a src/test/rustdoc test. Looking at existing tests, it appears // @has checks that a file or string is in the docs, and // @!has checks that a file or string is not in the docs.

r=me with a test

@jseyfried jseyfried mentioned this pull request Mar 25, 2017
Closed
@abonander
Copy link
Contributor Author

The second option isn't horrible I guess, I just don't like touching Resolver for a short-term fix. It feels like it's leaking the scope of the fix.

@bors
Copy link
Contributor

bors commented Mar 26, 2017

☔ The latest upstream changes (presumably #40826) made this pull request unmergeable. Please resolve the merge conflicts.

@abonander abonander changed the title Rustdoc: strip non-inlined pub use'd macros from the export map Rustdoc: memoize pub use-reexported macros so they don't appear twice in docs Mar 28, 2017
@abonander
Copy link
Contributor Author

@jseyfried New solution + test. I think you'll like it better than before.

@abonander
Copy link
Contributor Author

I've also added some blank lines in places because it makes those bits easier to read.

abonander
abonander commented Mar 28, 2017
View reviewed changes
src/librustdoc/visit_ast.rs Outdated

self.reexported_macros.insert(did);

return false;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could probably condense this block though, it looks weird in review.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd probably condense, but up to you.

@jseyfried
Copy link
Contributor

@abonander I do like this solution more :)
@bors r+
@bors delegate=abonander

@bors
Copy link
Contributor

bors commented Mar 28, 2017

✌️ @abonander can now approve this pull request

@bors
Copy link
Contributor

bors commented Mar 28, 2017

📌 Commit 9339522 has been approved by jseyfried

@abonander
Copy link
Contributor Author

@bors r=jseyfried

@bors
Copy link
Contributor

bors commented Mar 28, 2017

📌 Commit d8fc5b8 has been approved by jseyfried

@frewsxcv frewsxcv mentioned this pull request Mar 29, 2017
Closed
frewsxcv added a commit to frewsxcv/rust that referenced this pull request Mar 29, 2017
@frewsxcv
Rollup merge of rust-lang#40814 - abonander:issue_39436, r=jseyfried
873edbf 
Rustdoc: memoize `pub use`-reexported macros so they don't appear twice in docs

Closes rust-lang#39436

Preserves existing behavior for `#[macro_reexport]`. `pub use`'d macros are shown as reexports unless inlined, and also correctly obey `#[doc(hidden)]`.

r? @jseyfried

cc @SergioBenitez
@frewsxcv frewsxcv mentioned this pull request Mar 29, 2017
Closed
frewsxcv added a commit to frewsxcv/rust that referenced this pull request Mar 29, 2017
@frewsxcv
Rollup merge of rust-lang#40814 - abonander:issue_39436, r=jseyfried
ea9c8b9 
Rustdoc: memoize `pub use`-reexported macros so they don't appear twice in docs

Closes rust-lang#39436

Preserves existing behavior for `#[macro_reexport]`. `pub use`'d macros are shown as reexports unless inlined, and also correctly obey `#[doc(hidden)]`.

r? @jseyfried

cc @SergioBenitez
@frewsxcv frewsxcv mentioned this pull request Mar 29, 2017
Merged
bors added a commit that referenced this pull request Mar 29, 2017
@bors
Auto merge of #40911 - frewsxcv:rollup, r=frewsxcv
c82f132 
Rollup of 6 pull requests

- Successful merges: #40780, #40814, #40816, #40832, #40901, #40907
- Failed merges:
@bors bors merged commit d8fc5b8 into rust-lang:master Mar 30, 2017
@bors
Copy link
Contributor

bors commented Mar 30, 2017

⌛ Testing commit d8fc5b8 with merge c82f132...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jseyfried jseyfried jseyfried left review comments

Assignees

@jseyfried jseyfried

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@abonander @jseyfried @bors