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

Nr2 closure captures #3373

Merged
merged 2 commits into from
Jan 24, 2025
Merged

Nr2 closure captures #3373

merged 2 commits into from
Jan 24, 2025

Conversation

P-E-P
Copy link
Member

@P-E-P P-E-P commented Jan 20, 2025

Remove usage of nr1 resolver with closure captures.

Copy link
Contributor

@powerboat9 powerboat9 left a comment

Choose a reason for hiding this comment

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

Otherwise, looks great


DefaultResolver::visit (closure);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Would also have to handle ClosureExprInnerTyped -- maybe with some visit (static_cast <AST::ClosureExpr &> (closure)) shenanigans to avoid duplicating code?

Copy link
Member Author

@P-E-P P-E-P Jan 24, 2025

Choose a reason for hiding this comment

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

I used a simple templated function instead, I don't want anyone to be more confused, we've got 3 degrees of inheritance with this visitor, I already can't expect people to get it right the first time so I'd prefer if we could avoid a static upcast 😄 Thanks for noticing!

The compiler was still relying on NR1 for closure captures when using nr2
even though the resolver was not used and thus it's state empty.

gcc/rust/ChangeLog:

	* resolve/rust-late-name-resolver-2.0.cc (Late::visit): Add environment
	collection.
	* resolve/rust-late-name-resolver-2.0.h: Add function prototype.
	* resolve/rust-name-resolver.cc (Resolver::get_captures): Add assertion
	to prevent NR2 usage with nr1 capture functions.
	* typecheck/rust-hir-type-check-expr.cc (TypeCheckExpr::visit): Use
	nr2 captures.
	* util/rust-hir-map.cc (Mappings::add_capture): Add function to
	register capture for a given closure.
	(Mappings::lookup_captures):  Add a function to lookup all captures
	available for a given closure.
	* util/rust-hir-map.h: Add function prototypes.

Signed-off-by: Pierre-Emmanuel Patry <pierre-emmanuel.patry@embecosm.com>
@P-E-P P-E-P force-pushed the nr2-closure-captures branch from 11c48f0 to 409ca1b Compare January 24, 2025 14:31
Captures were only processed for regular ClosureExprInner.

gcc/rust/ChangeLog:

	* resolve/rust-late-name-resolver-2.0.cc (Late::visit): Add
	ClosureExprInnerTyped visit implementation.
	(add_captures): Add a function to avoid code duplication.
	* resolve/rust-late-name-resolver-2.0.h: Add function prototype.

Signed-off-by: Pierre-Emmanuel Patry <pierre-emmanuel.patry@embecosm.com>
@P-E-P
Copy link
Member Author

P-E-P commented Jan 24, 2025

I had to delete the commit with the now passing nr2 tests because they contain a multi segment TypePath.

@P-E-P P-E-P added this pull request to the merge queue Jan 24, 2025
Merged via the queue into Rust-GCC:master with commit 98d89d5 Jan 24, 2025
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

2 participants