Skip to content

update error E0451 to new format - #36054

Merged
bors merged 1 commit into
rust-lang:masterfrom
mikhail-m1:master2
Aug 30, 2016
Merged

update error E0451 to new format#36054
bors merged 1 commit into
rust-lang:masterfrom
mikhail-m1:master2

Conversation

@mikhail-m1

@mikhail-m1mikhail-m1 commented Aug 27, 2016

Copy link
Copy Markdown
Contributor

Fixes#35928 as part of #35233.

r? @jonathandturner

@rust-highfive

rust-highfive commented Aug 27, 2016

Copy link
Copy Markdown
Contributor

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @jonathandturner (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@sophiajt

sophiajt commented Aug 27, 2016

Copy link
Copy Markdown
Contributor

Thanks for the PR!

@bors r+ rollup

@bors

bors commented Aug 27, 2016

Copy link
Copy Markdown
Collaborator

📌 Commit a24ea73 has been approved by jonathandturner

@mikhail-m1

mikhail-m1 commented Aug 28, 2016

Copy link
Copy Markdown
ContributorAuthor

I broke compile-fail/functional-struct-update-respects-privacy.rs, trying to fix it

@GuillaumeGomezGuillaumeGomez mentioned this pull request Aug 29, 2016
Closed
TimNN
TimNN reviewed Aug 29, 2016
View reviewed changes
Comment threadsrc/librustc_privacy/lib.rs Outdated

@TimNNTimNNAug 29, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@mikhail-m1: The problem, I believe, is that fields contains less fields than variant.fields, which means that not all fields are iterated over.

Also I don't think it's guaranteed that the fields in fields and variant.fields occur in the same order - my intuition would be that variant.fields is in the order the fields were defined and fields is in the order the fields were written in the struct literal.

@mikhail-m1

mikhail-m1 commented Aug 29, 2016

Copy link
Copy Markdown
ContributorAuthor

librustc_llvm fails

@TimNN

TimNN commented Aug 29, 2016

Copy link
Copy Markdown
Contributor

The LLVM error is #36077, to be fixed by #36080.

TimNN
TimNN reviewed Aug 29, 2016
View reviewed changes
Comment threadsrc/librustc_privacy/lib.rs
} else {
expr.span
};
self.check_field(span, adt, field);

@TimNNTimNNAug 29, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think this should work now, however I'm a bit concerned that the complexity here went from linear to quadratic (for all struct literals). I'm not sure how much of an impact this will have on real world code however there are crates with structs with 1500+ fields in which case this may not be ideal.

@mikhail-m1mikhail-m1Aug 29, 2016

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I don't expect so big structs, but this code works only on matching expressions, does any one match more then tens fields?

@TimNNTimNNAug 29, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If I understand the situation correctly, this code runs for every struct literal, ie. let _ = Foo { bar: "hi!" }. And initialising such large structs does happen.

@mikhail-m1mikhail-m1Aug 29, 2016

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

ok, I'll rework it

@TimNN

TimNN commented Aug 29, 2016

Copy link
Copy Markdown
Contributor

I did some (absolutely non-scientific) performance measurements between local stage 1 builds (with and without this pr) and found no real differences in compile time, so the current implementation is probably fine.

@sophiajt

sophiajt commented Aug 29, 2016

Copy link
Copy Markdown
Contributor

@TimNN - thanks for checking. Sounds like we're good. Will get this moving again.

@bors r+ rollup

@bors

bors commented Aug 29, 2016

Copy link
Copy Markdown
Collaborator

📌 Commit 46fc80c has been approved by jonathandturner

@sophiajt

sophiajt commented Aug 29, 2016

Copy link
Copy Markdown
Contributor

@bors r-

I missed the "I broke compile-fail/functional-struct-update-respects-privacy.rs, trying to fix it" comment. Let me know when this gets fixed and I'll put it back through review.

@TimNN

TimNN commented Aug 29, 2016

Copy link
Copy Markdown
Contributor

@jonathandturner: That should habe been fixed by the updated commit as well.

@sophiajt

sophiajt commented Aug 29, 2016

Copy link
Copy Markdown
Contributor

@TimNN - Oh okay. With Travis down for the moment, we're having to guess what will pass based on what failed in previous builds. If you think it's ready, I can re:r+ it

@TimNN

TimNN commented Aug 29, 2016

Copy link
Copy Markdown
Contributor

@jonathandturner - I'm confident that the previous failure of compile-fail/functional-struct-update-respects-privacy.rs has been fixed. Also since there is no other NOTE matched in that test, the test should be fine as well.

As such I think this pr is good to go.

@sophiajt

sophiajt commented Aug 29, 2016

Copy link
Copy Markdown
Contributor

@TimNN - great, thanks for checking

@bors r+ rollup

@bors

bors commented Aug 29, 2016

Copy link
Copy Markdown
Collaborator

📌 Commit 46fc80c has been approved by jonathandturner

bors added a commit that referenced this pull request Aug 29, 2016
@bors
Auto merge of#36090- GuillaumeGomez:rollup, r=GuillaumeGomez
aee9774
Rollup of 11 pull requests
- Successful merges: #35418, #35759, #35862, #35863, #35962, #35977, #35993, #35997, #36054, #36056, #36070
- Failed merges: #35771, #35810
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Aug 30, 2016
@GuillaumeGomez
Rollup merge ofrust-lang#36054- mikhail-m1:master2, r=jonathandturner
9c758f3
update error E0451 to new format
Fixesrust-lang#35928 as part of rust-lang#35233.
r? @jonathandturner
@GuillaumeGomezGuillaumeGomez mentioned this pull request Aug 30, 2016
Merged
bors added a commit that referenced this pull request Aug 30, 2016
@bors
Auto merge of#36126- GuillaumeGomez:rollup, r=GuillaumeGomez
badf406
Rollup of 16 pull requests
- Successful merges: #35418, #35759, #35862, #35863, #35895, #35962, #35977, #35993, #35997, #36054, #36056, #36060, #36086, #36100, #36103, #36125
- Failed merges: #35771, #35810
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Aug 30, 2016
@GuillaumeGomez
Rollup merge ofrust-lang#36054- mikhail-m1:master2, r=jonathandturner
5185bd0
update error E0451 to new format
Fixesrust-lang#35928 as part of rust-lang#35233.
r? @jonathandturner
bors added a commit that referenced this pull request Aug 30, 2016
@bors
Auto merge of#36126- GuillaumeGomez:rollup, r=GuillaumeGomez
4473130
Rollup of 16 pull requests
- Successful merges: #35418, #35759, #35862, #35863, #35895, #35962, #35977, #35993, #35997, #36054, #36056, #36060, #36086, #36100, #36103, #36125
- Failed merges: #35771, #35810
@bors
bors merged commit 46fc80c into rust-lang:masterAug 30, 2016
@mikhail-m1
mikhail-m1 deleted the master2 branch November 30, 2016 20:48
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@sophiajtsophiajt

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@mikhail-m1@rust-highfive@sophiajt@bors@TimNN