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.

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

implement feature(const_generics_defaults) #75384

Merged
merged 8 commits into from
Mar 24, 2021
Merged

Conversation

JulianKnodt
Copy link
Contributor

@JulianKnodt JulianKnodt commented Aug 11, 2020

Implements const generics defaults struct Example<const N: usize=3>, as well as a query for getting the default of a given const-parameter's def id. There are some remaining FIXME's but they were specified as not blocking for merging this PR. This also puts the defaults behind the unstable feature gate #![feature(const_generics_defaults)].

This currently creates a field which is always false on GenericParamDefKind for future use when
consts are permitted to have defaults. I'm not sure if this is exactly what is best for adding default parameters, but I mimicked the style of type defaults, so hopefully this is ok.

r? @lcnr

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 11, 2020
@bors
Copy link
Contributor

bors commented Aug 11, 2020

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

@lcnr
Copy link
Contributor

lcnr commented Aug 11, 2020

Hmm, I am not sure how easy it is to start here and think that's it's probably better to start by actually parsing const N: Type = expr, so we can test these changes.

Do we want to implement this as part of const_generics or do we want a new feature gate for this?

Also, while I will review this, I won't merge it myself, so r? @varkor

@JulianKnodt JulianKnodt force-pushed the cg_def branch 2 times, most recently from ce46be1 to 705cbf7 Compare August 11, 2020 07:43
@varkor
Copy link
Member

varkor commented Aug 11, 2020

I think if we're going to make any progress with defaults for const generics, we ought to try to implement them in a single PR, or at least bigger steps than this. Otherwise we risk making implementation choices that aren't the best in practice, and then refactoring unnecessarily later. You're welcome to try implementing defaults, though: I think we'll want an extra feature flag const_generics_defaults, because it's likely we won't stabilise them along with the rest of const generics (they weren't part of the original RFC). There are lots of FIXME(const_generics:defaults) throughout the code that should pinpoint most of the places that need changing accordingly.

@JulianKnodt
Copy link
Contributor Author

JulianKnodt commented Aug 14, 2020

Hm I think I added all the points where I saw the FIXME(const_generics:defaults), but as this would be a larger change maybe I'll hold off for a bit

@varkor
Copy link
Member

varkor commented Aug 14, 2020

Oh, sorry, I hadn't actually looked at the changes yet. Maybe this actually isn't too far off a minimal working implementation? Maybe if you tried implementing the parser changes, we could get a better idea of what does or doesn't work?

@varkor varkor added the F-const_generics `#![feature(const_generics)]` label Aug 14, 2020
src/librustc_ast/ast.rs Outdated Show resolved Hide resolved
@JulianKnodt JulianKnodt force-pushed the cg_def branch 3 times, most recently from 34c40c6 to 052e043 Compare August 15, 2020 00:20
@varkor varkor 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 Aug 16, 2020
@bors
Copy link
Contributor

bors commented Aug 17, 2020

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

@JulianKnodt
Copy link
Contributor Author

Hm I'm a little stuck at this point, I'm not particularly sure where the error could be originating from.

@JohnCSimon
Copy link
Member

@JulianKnodt - ping from triage, can you please address the merge conflicts?

@JulianKnodt JulianKnodt force-pushed the cg_def branch 2 times, most recently from 6c0b4ca to 2ab5e69 Compare September 1, 2020 21:31
@bors
Copy link
Contributor

bors commented Mar 23, 2021

📌 Commit 33370fd has been approved by varkor,lcnr

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 23, 2021
@Dylan-DPC-zz
Copy link

@bors retry

@bors
Copy link
Contributor

bors commented Mar 23, 2021

⌛ Testing commit 33370fd with merge c964c9ae35ee07abd1712a8f902a6f3852d059bb...

@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@bors
Copy link
Contributor

bors commented Mar 24, 2021

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 24, 2021
@varkor
Copy link
Member

varkor commented Mar 24, 2021

@bors retry

@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 Mar 24, 2021
@bors
Copy link
Contributor

bors commented Mar 24, 2021

⌛ Testing commit 33370fd with merge 5b33de3...

@bors
Copy link
Contributor

bors commented Mar 24, 2021

☀️ Test successful - checks-actions
Approved by: varkor,lcnr
Pushing 5b33de3 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 24, 2021
@bors bors merged commit 5b33de3 into rust-lang:master Mar 24, 2021
@rustbot rustbot added this to the 1.53.0 milestone Mar 24, 2021
@varkor
Copy link
Member

varkor commented Mar 24, 2021

@JulianKnodt: thank you for all your hard work pushing this feature to completion!

@rylev
Copy link
Member

rylev commented Apr 1, 2021

@JulianKnodt @varkor - there was a slight performance regression seen after a performance run of this change. It's fairly small and it only impacts one benchmark which does not use const generics at all. The specific query impacted is generate_crate_metadata, but from the changes it doesn't look like metadata encoding changed very much at all. Any ideas?

@JulianKnodt
Copy link
Contributor Author

Probably CGU partitioning changed after the queries changed?

@mati865
Copy link
Contributor

mati865 commented Apr 4, 2021

generate_crate_metadata takes longer now, I doubt it's related to CGU partitioning.

I'd also say this is expected perf change given what this does.

@lcnr lcnr added the A-const-generics Area: const generics (parameters and arguments) label Aug 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-const-generics Area: const generics (parameters and arguments) F-const_generics `#![feature(const_generics)]` 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
Development

Successfully merging this pull request may close these issues.