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

Validate rustc_args_required_const #77343

Merged
merged 1 commit into from
Oct 1, 2020
Merged

Validate rustc_args_required_const #77343

merged 1 commit into from
Oct 1, 2020

Conversation

varkor
Copy link
Member

@varkor varkor commented Sep 29, 2020

Fixes #74608.

@rust-highfive
Copy link
Collaborator

r? @lcnr

(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 Sep 29, 2020
@varkor varkor force-pushed the rustc_args_required_const-validation branch from cecc871 to d40e652 Compare September 29, 2020 17:02
lcnr
lcnr approved these changes Sep 29, 2020
View reviewed changes
Copy link
Contributor

@lcnr lcnr left a comment

Choose a reason for hiding this comment

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

a small nit, otherwise LGTM

@varkor varkor force-pushed the rustc_args_required_const-validation branch from d40e652 to 5156c2b Compare September 29, 2020 20:45
lcnr
lcnr reviewed Sep 29, 2020
View reviewed changes
compiler/rustc_passes/src/check_attr.rs Outdated
if let Some(item) = item {
match &item.kind {
ItemKind::Fn(sig, ..) => {
if val >= sig.decl.inputs.len() as u128 {
Copy link
Contributor

Choose a reason for hiding this comment

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

this is a really deep indentation,

Can you convert some branches into early exists instead?

r=me after that

@varkor
Copy link
Member Author

varkor commented Sep 29, 2020

I've just noticed this isn't validating foreign functions properly; I'll fix that soon.

@lcnr
Copy link
Contributor

lcnr commented Sep 30, 2020

@varkor a PR by @davidtwco will make this obsolete, so it's fine to merge this without checks on extern functions as #77015 will add them automatically

@varkor varkor force-pushed the rustc_args_required_const-validation branch from 5156c2b to ebba2a4 Compare September 30, 2020 10:46
@varkor
Copy link
Member Author

varkor commented Sep 30, 2020

I took a look at that PR, but it didn't seem to cover this case. I've fixed the handling of ForeignItem now.

@varkor varkor force-pushed the rustc_args_required_const-validation branch from ebba2a4 to 609786d Compare September 30, 2020 10:59
@lcnr
Copy link
Contributor

lcnr commented Sep 30, 2020

Yeah, mixed something up here, #77015 looks at variants, the problem with extern items is something else I recently interacted with (https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/visit.20.60ForeignItem.60s.20in.20.60ItemLikeVisitor.60/near/210605191)

LGTM

@bors r+

@bors
Copy link
Contributor

bors commented Sep 30, 2020

📌 Commit 609786d has been approved by 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-review Status: Awaiting review from the assignee but also interested parties. labels Sep 30, 2020
@jonas-schievink jonas-schievink mentioned this pull request Sep 30, 2020
Closed
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Sep 30, 2020
@Dylan-DPC
Rollup merge of rust-lang#77343 - varkor:rustc_args_required_const-va…
b70cb3d 
…lidation, r=lcnr

Validate `rustc_args_required_const`

Fixes rust-lang#74608.
@Dylan-DPC-zz Dylan-DPC-zz mentioned this pull request Sep 30, 2020
Closed
@Dylan-DPC-zz Dylan-DPC-zz mentioned this pull request Oct 1, 2020
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 1, 2020
@bors
Auto merge of rust-lang#77381 - Dylan-DPC:rollup-0sr6p5p, r=Dylan-DPC
b218b95 
Rollup of 12 pull requests

Successful merges:

 - rust-lang#76909 (Add Iterator::advance_by and DoubleEndedIterator::advance_back_by)
 - rust-lang#77153 (Fix recursive nonterminal expansion during pretty-print/reparse check)
 - rust-lang#77202 (Defer Apple SDKROOT detection to link time.)
 - rust-lang#77303 (const evaluatable: improve `TooGeneric` handling)
 - rust-lang#77305 (move candidate_from_obligation_no_cache)
 - rust-lang#77315 (Rename AllocErr to AllocError)
 - rust-lang#77319 (Stable hashing: add comments and tests concerning platform-independence)
 - rust-lang#77324 (Don't fire `const_item_mutation` lint on writes through a pointer)
 - rust-lang#77343 (Validate `rustc_args_required_const`)
 - rust-lang#77349 (Update cargo)
 - rust-lang#77360 (References to ZSTs may be at arbitrary aligned addresses)
 - rust-lang#77371 (Remove trailing space in error message)

Failed merges:

r? `@ghost`
@bors bors merged commit 849e563 into rust-lang:master Oct 1, 2020
@rustbot rustbot added this to the 1.48.0 milestone Oct 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lcnr lcnr lcnr approved these changes

Assignees

@lcnr lcnr

Labels
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.48.0
Development

Successfully merging this pull request may close these issues.

rustc_args_required_const ICE with missing parameter
5 participants
@varkor @rust-highfive @lcnr @bors @rustbot