Skip to content

Remove NodeId from Block and Expr - #58698

Closed
ljedrz wants to merge 12 commits into
rust-lang:masterfrom
ljedrz:HirIdify_more_nodes
Closed

Remove NodeId from Block and Expr#58698
ljedrz wants to merge 12 commits into
rust-lang:masterfrom
ljedrz:HirIdify_more_nodes

Conversation

@ljedrz

Copy link
Copy Markdown
Contributor

The next iteration of #57578. The relevant part is the last 2 commits (the others are dependencies).

Removes NodeId from

  • hir::Block
  • hir::Expr

Blocked by #58561, can be appended to it.

r? @Zoxc

@rust-highfiverust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 24, 2019
@ljedrzljedrz mentioned this pull request Feb 24, 2019
@ljedrzljedrz changed the title HirIdify more nodesRemove NodeId from Block and ExprFeb 24, 2019
@ljedrz

Copy link
Copy Markdown
ContributorAuthor

Oof, I forgot about the Expr size assertion; I'll update shortly.


let mut unsafe_blocks: Vec<_> = unsafe_blocks.into_iter().collect();
unsafe_blocks.sort();
unsafe_blocks.sort_by_cached_key(|(hir_id, _)| tcx.hir().hir_to_node_id(*hir_id));

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 it's find to just use sort here. Do you know which errors changed order with that?

@ljedrzljedrzFeb 24, 2019

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.

Nope, plain sort breaks 2 issue test cases related to unused unsafe (unfortunately I forgot to register which ones).

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.

Well we can land sort_by_cached_key here and you could open a new PR with an additional commit which changes it back to sort, so we can look at the failures.

s.s.space()?;
s.synth_comment(format!("node_id: {} hir local_id: {}",
expr.id, expr.hir_id.local_id.as_u32()))?;
s.synth_comment(format!("expr hir_id: {} hir local_id: {}",

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.

You added expr here. Not sure if that's good or bad =P

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.

Yes, so that it matches the other branches; I mean, it shouldn't break anything ^^.

Node::Expr(expr) => {
return (ty::Visibility::Restricted(tcx.hir().get_module_parent(expr.id)),
return (ty::Visibility::Restricted(
tcx.hir().get_module_parent_by_hir_id(expr.hir_id)),

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.

This formatting doesn't quite make sense to me =P

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 think I was just trying to squeeze it so that tidy doesn't complain; I can adjust it if need be :P.

@Zoxc

Zoxc commented Feb 24, 2019

Copy link
Copy Markdown
Contributor

I've looked over this and it looks good. You can append it to #58561

@ljedrz

Copy link
Copy Markdown
ContributorAuthor

@Zoxc done; what about that static_assert on Expr, though? I forgot to alter it, but it doesn't seem to break anything.

@ljedrz

Copy link
Copy Markdown
ContributorAuthor

Oh, and closing in favor of #58561.

@ljedrzljedrz closed this Feb 24, 2019
@Zoxc

Zoxc commented Feb 24, 2019

Copy link
Copy Markdown
Contributor

@ljedrz That's probably due to padding.

@ljedrz

Copy link
Copy Markdown
ContributorAuthor

@Zoxc I ran tests with that plain sort locally; the results are as follows:

failures:
---- [ui] ui\issues\issue-45107-unnecessary-unsafe-in-closure.rs stdout ----
diff of stderr:
1 error: unnecessary `unsafe` block
- --> $DIR/issue-45107-unnecessary-unsafe-in-closure.rs:7:13
+ --> $DIR/issue-45107-unnecessary-unsafe-in-closure.rs:9:38
3 |
4 LL | unsafe {
5 | ------ because it's nested under this `unsafe` block
- LL | let f = |v: &mut Vec<_>| {
- LL | unsafe { //~ ERROR unnecessary `unsafe`
- | ^^^^^^ unnecessary `unsafe` block
+ ...
+ LL | |w: &mut Vec<u32>| { unsafe { //~ ERROR unnecessary `unsafe`
+ | ^^^^^^ unnecessary `unsafe` block
9 |
10 note: lint level defined here
11 --> $DIR/issue-45107-unnecessary-unsafe-in-closure.rs:1:8
14 | ^^^^^^^^^^^^^
15
16 error: unnecessary `unsafe` block
- --> $DIR/issue-45107-unnecessary-unsafe-in-closure.rs:9:38
+ --> $DIR/issue-45107-unnecessary-unsafe-in-closure.rs:7:13
18 |
19 LL | unsafe {
20 | ------ because it's nested under this `unsafe` block
- ...
- LL | |w: &mut Vec<u32>| { unsafe { //~ ERROR unnecessary `unsafe`
- | ^^^^^^ unnecessary `unsafe` block
+ LL | let f = |v: &mut Vec<_>| {
+ LL | unsafe { //~ ERROR unnecessary `unsafe`
+ | ^^^^^^ unnecessary `unsafe` block
24
25 error: unnecessary `unsafe` block
26 --> $DIR/issue-45107-unnecessary-unsafe-in-closure.rs:13:34
---- [ui] ui\span\lint-unused-unsafe.rs stdout ----
diff of stderr:
47 | ^^^^^^ unnecessary `unsafe` block
48
49 error: unnecessary `unsafe` block
- --> $DIR/lint-unused-unsafe.rs:29:5
+ --> $DIR/lint-unused-unsafe.rs:30:9
51 |
52 LL | unsafe fn bad7() {
53 | ---------------- because it's nested under this `unsafe` fn
54 LL | unsafe { //~ ERROR: unnecessary `unsafe` block
- | ^^^^^^ unnecessary `unsafe` block
+ LL | unsafe { //~ ERROR: unnecessary `unsafe` block
+ | ^^^^^^ unnecessary `unsafe` block
56
57 error: unnecessary `unsafe` block
- --> $DIR/lint-unused-unsafe.rs:30:9
+ --> $DIR/lint-unused-unsafe.rs:29:5
59 |
60 LL | unsafe fn bad7() {
61 | ---------------- because it's nested under this `unsafe` fn
62 LL | unsafe { //~ ERROR: unnecessary `unsafe` block
- LL | unsafe { //~ ERROR: unnecessary `unsafe` block
- | ^^^^^^ unnecessary `unsafe` block
+ | ^^^^^^ unnecessary `unsafe` block
65
66 error: aborting due to 8 previous errors
67
failures:
[ui] ui\issues\issue-45107-unnecessary-unsafe-in-closure.rs
[ui] ui\span\lint-unused-unsafe.rs

@ljedrz
ljedrz deleted the HirIdify_more_nodes branch February 26, 2019 16:52
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-reviewStatus: Awaiting review from the assignee but also interested parties.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@ljedrz@Zoxc@rust-highfive