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

extend WithOptConstParam docs, move rustdoc test #78609

Merged
merged 2 commits into from
Dec 6, 2020
Merged

extend WithOptConstParam docs, move rustdoc test #78609

merged 2 commits into from
Dec 6, 2020

Conversation

lcnr
Copy link
Contributor

@lcnr lcnr commented Oct 31, 2020

This should hopefully make things a bit clearer, feel free to comment on anything which can still be improved.

cc @ecstatic-morse @nikomatsakis @RalfJung

@rust-highfive
Copy link
Collaborator

r? @matthewjasper

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 31, 2020
RalfJung
RalfJung reviewed Nov 7, 2020
View reviewed changes
compiler/rustc_middle/src/ty/mod.rs Outdated
///
/// We only care about the const argument once we know which paramter they instantiate.
/// So instead of using `type_of(const_arg)` we can use `type_of(const_param)` which does
/// not depend on `typeck`.
Copy link
Member

Choose a reason for hiding this comment

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

typeck of what?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

on any typeck. type_of(const_param) is fairly trivial as it is directly mentioned by the user.

We have const N: usize and the type_of(N) should return usize

Copy link
Member

@RalfJung RalfJung Nov 7, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I cannot make sense of this sentence. Above, you use things like typeck(main).

You seem to assume that the reader knows the half a dozen queries involved in this and how they related, and maybe that's fair, but I know none of them.^^

If that makes me not part of the audience for these docs, that's okay; in that case just write something that you think make sense and do not wait for me to understand it all. :)

@Dylan-DPC-zz
Copy link

@lcnr this is waiting for you to resolve CI issues + the above review

@matthewjasper matthewjasper added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 28, 2020
@lcnr
Copy link
Contributor Author

lcnr commented Dec 1, 2020

@RalfJung thought about it a bit more and hope that it at least doesn't look completely nonsensical to you anymore.

It still requires some knowledge about how the type system works, but I prefer merging this PR without spending much additional time on this.

@RalfJung
Copy link
Member

RalfJung commented Dec 2, 2020

Even if there are still some details I am fuzzy on, I think this PR is a definite improvement, so I am fine with landing it.

@lcnr lcnr added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 3, 2020
@matthewjasper
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Dec 6, 2020

📌 Commit 4b23f40 has been approved by matthewjasper

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 6, 2020
@bors
Copy link
Contributor

bors commented Dec 6, 2020

⌛ Testing commit 4b23f40 with merge d577c53...

@bors
Copy link
Contributor

bors commented Dec 6, 2020

☀️ Test successful - checks-actions
Approved by: matthewjasper
Pushing d577c53 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 6, 2020
@bors bors merged commit d577c53 into rust-lang:master Dec 6, 2020
@rustbot rustbot added this to the 1.50.0 milestone Dec 6, 2020
@lcnr lcnr added the A-const-generics Area: const generics (parameters and arguments) label Dec 11, 2021
@lcnr lcnr mentioned this pull request Dec 11, 2021
Closed
16 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matthewjasper matthewjasper matthewjasper left review comments

@RalfJung RalfJung RalfJung left review comments

Assignees

@matthewjasper matthewjasper

Labels
A-const-generics Area: const generics (parameters and arguments) 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
1.50.0
Development

Successfully merging this pull request may close these issues.
7 participants
@lcnr @rust-highfive @Dylan-DPC-zz @RalfJung @matthewjasper @bors @rustbot