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

Introduce lowering_arena to avoid creating AST nodes on the fly #101499

Merged
merged 1 commit into from
Sep 9, 2022
Merged

Introduce lowering_arena to avoid creating AST nodes on the fly #101499

merged 1 commit into from
Sep 9, 2022

Conversation

spastorino
Copy link
Member

@oli-obk requested this and other changes as a way of simplifying #101345. This is just going to make the diff of #101345 smaller.

r? @oli-obk @cjgillot

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Sep 6, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 6, 2022
cjgillot
cjgillot reviewed Sep 6, 2022
View reviewed changes
Copy link
Contributor

@cjgillot cjgillot left a comment

Choose a reason for hiding this comment

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

Just one nit. r=me afterwards.

compiler/rustc_ast_lowering/src/lib.rs Outdated
@@ -140,6 +148,12 @@ struct LoweringContext<'a, 'hir> {
generics_def_id_map: Vec<FxHashMap<LocalDefId, LocalDefId>>,
}

struct LoweringArena {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you use rustc_arena::declare_arena?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor

Choose a reason for hiding this comment

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

I meant using the macro rustc_arena::declare_arena.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've tried to use declare_arena but that imposes a 'tcx lifetime which any of the fields have. At that point I've tried _marker: PhantomData<&'tcx ()> but then I got a bunch of lifetime errors and I ended thinking that using the macro may be more complicated than writing this manually.
Let me know if you still want to persue this or this is ready to be merged in the way it is.

@spastorino spastorino requested a review from cjgillot September 6, 2022 19:35
@oli-obk
Copy link
Contributor

oli-obk commented Sep 7, 2022

r? @cjgillot

@rust-highfive rust-highfive assigned cjgillot and unassigned oli-obk Sep 7, 2022
@spastorino
Copy link
Member Author

Just one nit. r=me afterwards.

@bors r=cjgillot

@bors
Copy link
Contributor

bors commented Sep 7, 2022

📌 Commit 2df3743a2a4b3bcbdfeb98d4aca2d543e10e6881 has been approved by cjgillot

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Sep 7, 2022

🌲 The tree is currently closed for pull requests below priority 1000. This pull request will be tested once the tree is reopened.

@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 7, 2022
@spastorino
Copy link
Member Author

@bors rollup

@spastorino
Copy link
Member Author

@bors r-

I've figured that I misunderstood @cjgillot comment, gonna address the problem in a bit

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 7, 2022
@spastorino
Copy link
Member Author

I think this should be merged as is, commented on the problem with declare_arena in #101499 (comment)

@spastorino spastorino 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 Sep 8, 2022
@cjgillot
Copy link
Contributor

cjgillot commented Sep 8, 2022

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Sep 8, 2022

📌 Commit ea097476baa4eacb13715439565e34dac73a4c01 has been approved by cjgillot

It is now in the queue for this repository.

@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 8, 2022
@spastorino
Copy link
Member Author

spastorino commented Sep 8, 2022 •
edited
Loading

@cjgillot ended doing it with the macro, needed PhantomData on the definition and 'static on the call site. Let me know if this version is better for you or the old one.

Edit: sorry that I didn't see you have already r+ed. Let me know which version do you prefer and I can r=cjgillot

@spastorino
Copy link
Member Author

@oli-obk said "leave a comment on the PhanomData field and r=me"

@bors r=oli-obk rollup

@bors
Copy link
Contributor

bors commented Sep 8, 2022

📌 Commit d9a1faa has been approved by oli-obk

It is now in the queue for this repository.

@compiler-errors compiler-errors mentioned this pull request Sep 8, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 8, 2022
@bors
Auto merge of rust-lang#101592 - compiler-errors:rollup-d2fya7z, r=co…
c6861df 
…mpiler-errors

Rollup of 7 pull requests

Successful merges:

 - rust-lang#101423 (Fix hermit warnings)
 - rust-lang#101499 (Introduce lowering_arena to avoid creating AST nodes on the fly)
 - rust-lang#101530 (llvm-wrapper: adapt for LLVM API changes)
 - rust-lang#101554 (rustdoc: remove unused CSS `#implementations-list > h3 > span.in-band`)
 - rust-lang#101580 (rustdoc: remove unused CSS `div.impl-items > div`)
 - rust-lang#101584 (rustdoc: remove no-op CSS `#settings-menu { padding: 0 }`)
 - rust-lang#101587 (Make `Debug` impl for `Term` useful)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 1499c08 into rust-lang:master Sep 9, 2022
@rustbot rustbot added this to the 1.65.0 milestone Sep 9, 2022
@Noratrieb Noratrieb mentioned this pull request Dec 18, 2022
Merged
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 18, 2022
@matthiaskrgr
Rollup merge of rust-lang#105879 - Nilstrieb:revert-hir-arena, r=oli-obk
87a4694 
Revert "Introduce lowering_arena to avoid creating AST nodes on the fly"

This reverts commit d9a1faa (rust-lang#101499).

This was originally part of rust-lang#101345 which has now been closed as a different approach is taken now.

r? `@oli-obk`

cc `@spastorino`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjgillot cjgillot Awaiting requested review from cjgillot

Assignees

@cjgillot cjgillot

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.65.0
Development

Successfully merging this pull request may close these issues.
6 participants
@spastorino @oli-obk @bors @cjgillot @rust-highfive @rustbot