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

Fix display of const generics in rustdoc #60760

Merged
merged 1 commit into from
May 19, 2019
Merged

Fix display of const generics in rustdoc #60760

merged 1 commit into from
May 19, 2019

Conversation

GuillaumeGomez
Copy link
Member

@GuillaumeGomez GuillaumeGomez commented May 12, 2019 •
edited
Loading

Screenshot 2019-05-18 at 15 45 22

Part of #60737.

cc @varkor

r? @badboy

@varkor
Copy link
Member

varkor commented May 12, 2019

Instead of impl<const ORDER: Order, T> Send for VSet<T, const ORDER: Order>, it should be impl<const ORDER: Order, T> Send for VSet<T, ORDER> (the const and type aren't present in the arguments).

badboy
badboy suggested changes May 13, 2019
View reviewed changes
Copy link
Member

@badboy badboy left a comment

Choose a reason for hiding this comment

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

Heads-up: It's the first time I'm reviewing anything here, I'm not familiar with the rustdoc codebase.

See my inline comment regarding the fixme.
I do agree with varkor for the display.

src/librustdoc/clean/mod.rs Outdated
expr: format!("{:?}", self.val), // FIXME(const_generics)
expr: match self.val {
ConstValue::Param(ty::ParamConst { name, .. }) => format!("const {}", name),
e => format!("{:?}", e), // FIXME generic consts
Copy link
Member

Choose a reason for hiding this comment

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

I don't think it's entirely clear what's still to fix.

Copy link
Member

@varkor varkor May 13, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

In general, the constant could be an arbitrary expression, in which case we want to pretty-print it rather than debug format it. (This could be made more clear in the comment.)

src/test/rustdoc/generic-const.rs
Unsorted,
}

// @has foo/struct.VSet.html '//pre[@class="rust struct"]' 'pub struct VSet<T, const ORDER: Order>'
Copy link
Member

Choose a reason for hiding this comment

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

TIL: rustdoc tests

@GuillaumeGomez
Copy link
Member Author

I'll update with @varkor's comments!

@GuillaumeGomez
Copy link
Member Author

Updated!

@varkor
Copy link
Member

varkor commented May 18, 2019

Looks good, thanks! r=me unless @badboy has any additional comments.

@GuillaumeGomez
Copy link
Member Author

Thanks for both of your reviews! :)

@bors: r=varkor,badboy

@bors
Copy link
Contributor

bors commented May 18, 2019

📌 Commit 2caeaf5 has been approved by varkor,badboy

@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label May 18, 2019
@Centril
Copy link
Contributor

Centril commented May 19, 2019

@varkor Why is the const parameter before the type one in the auto traits?

@GuillaumeGomez Also, I think it would be nice to do away with the unnecessary braces around the const arguments.

@varkor
Copy link
Member

varkor commented May 19, 2019

Why is the const parameter before the type one in the auto traits?

Hmm, I'm not sure. But it doesn't seem related to this change.

Also, I think it would be nice to do away with the unnecessary braces around the const arguments.

Writing it without curly brackets doesn't currently work.

@Centril
Copy link
Contributor

Centril commented May 19, 2019

Writing it without curly brackets doesn't currently work.

@varkor Yeah but you could normalize in the rustdoc display itself.

@varkor
Copy link
Member

varkor commented May 19, 2019

That would not represent the actual syntax. When the issue with parameter names is fixed, then this will no longer be a problem.

@bors
Copy link
Contributor

bors commented May 19, 2019

⌛ Testing commit 2caeaf5 with merge e0d2f74...

bors added a commit that referenced this pull request May 19, 2019
@bors
Auto merge of #60760 - GuillaumeGomez:generic-display, r=varkor,badboy
e0d2f74 
Fix display of const generics in rustdoc

<img width="745" alt="Screenshot 2019-05-18 at 15 45 22" src="https://user-images.githubusercontent.com/3050060/57970638-04854e80-7984-11e9-9f04-da6b51ec8bc7.png">

Part of #60737.

cc @varkor

r? @badboy
@bors
Copy link
Contributor

bors commented May 19, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: varkor,badboy
Pushing e0d2f74 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 19, 2019
@bors bors merged commit 2caeaf5 into rust-lang:master May 19, 2019
@GuillaumeGomez GuillaumeGomez deleted the generic-display branch May 19, 2019 08:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@varkor varkor varkor left review comments

@badboy badboy badboy approved these changes

Assignees
No one assigned
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@GuillaumeGomez @varkor @bors @Centril @badboy