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

Remove references to smart pointers in the AST API #2867

Merged
merged 2 commits into from
Mar 13, 2024
Merged

Remove references to smart pointers in the AST API #2867

merged 2 commits into from
Mar 13, 2024

Conversation

P-E-P
Copy link
Member

@P-E-P P-E-P commented Feb 22, 2024

Remove references to smart pointer as those could be replaced with references to the underlying object.

@P-E-P P-E-P added the cleanup label Feb 22, 2024
@P-E-P P-E-P added this to the GCC 14.1 release milestone Feb 22, 2024
@P-E-P P-E-P requested a review from CohenArthur February 22, 2024 18:02
@P-E-P P-E-P self-assigned this Feb 22, 2024
powerboat9
powerboat9 reviewed Feb 22, 2024
View reviewed changes
gcc/rust/expand/rust-expand-visitor.cc Outdated
expander.pop_context ();

auto final_fragment = expander.take_expanded_fragment ();
if (final_fragment.should_expand ()
&& final_fragment.is_expression_fragment ())
expr = final_fragment.take_expression_fragment ();
Copy link
Collaborator

Choose a reason for hiding this comment

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

It doesn't seem like this would work, since instances of derived classes of Expr can have different sizes

Copy link
Member Author

@P-E-P P-E-P Feb 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

You're right. I believe the only way out is to keep reference to the unique pointer for type/expr expansion.

We could also return the unique_ptr after taking it but it looks more convoluted, I'll keep the first changes.

@P-E-P P-E-P marked this pull request as draft February 23, 2024 11:11
@P-E-P P-E-P mentioned this pull request Feb 27, 2024
Merged
@P-E-P P-E-P force-pushed the no-more-ref-to-sp branch from ac21b32 to c4a39be Compare February 27, 2024 15:07
@P-E-P P-E-P marked this pull request as ready for review February 27, 2024 16:44
CohenArthur
CohenArthur approved these changes Mar 7, 2024
View reviewed changes
Copy link
Member

@CohenArthur CohenArthur left a comment

Choose a reason for hiding this comment

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

LGTM :) I think this needs to be rebased

@P-E-P
Replace reference to unique pointer with reference
63803bd 
Reference to unique pointers are a known anti pattern, only the element
shall be taken by reference instead of the whole wrapper.

gcc/rust/ChangeLog:

	* ast/rust-item.h: Change getter function prototype to return a
	reference directly instead of a reference to the wrapper type.
	* checks/errors/rust-ast-validation.cc (ASTValidation::visit): Fix
	the code to accept references instead.
	* hir/rust-ast-lower-base.cc (ASTLoweringBase::lower_self): Change
	function implementation to return a reference.
	* hir/rust-ast-lower-base.h: Accept a reference instead of a unique
	pointer reference.
	* resolve/rust-ast-resolve-item.cc (ResolveItem::visit): Adapt the code
	to a reference instead of a unique pointer.

Signed-off-by: Pierre-Emmanuel Patry <pierre-emmanuel.patry@embecosm.com>
@P-E-P P-E-P force-pushed the no-more-ref-to-sp branch from c4a39be to 137f8c1 Compare March 8, 2024 15:52
@P-E-P
Replace unique_ptr references with references
e8cc906 
This kind of double indirection is pointless and prone to error. This
commit change the api of all getters from the AST to use references
directly instead of references to unique pointers.

gcc/rust/ChangeLog:

	* ast/rust-ast-collector.cc (TokenCollector::visit): Remove reference
	to unique pointer and replace it with a direct reference to the wrapped
	data.
	* ast/rust-ast.cc (VariadicParam::as_string): Likewise.
	(BlockExpr::normalize_tail_expr): Likewise.
	* ast/rust-expr.h: Likewise and add pointer getter in order to allow
	pointer reseat.
	* ast/rust-item.h: Likewise and add pointer getter for reseat.
	* ast/rust-path.h: Likewise.
	* ast/rust-pattern.h: Likewise.
	* ast/rust-stmt.h: Likewise.
	* ast/rust-type.h: Likewise.
	* expand/rust-cfg-strip.cc (CfgStrip::maybe_strip_struct_fields):
	Remove references to unique pointers and replace it with direct
	references to the wrapped object.
	(CfgStrip::maybe_strip_tuple_fields): Likewise.
	(CfgStrip::maybe_strip_generic_args): Likewise.
	(CfgStrip::maybe_strip_qualified_path_type): Likewise.
	(CfgStrip::visit): Likewise.
	* expand/rust-expand-visitor.cc (ExpandVisitor::maybe_expand_expr):
	Likewise.
	(ExpandVisitor::maybe_expand_type): Likewise.
	(ExpandVisitor::visit): Likewise.
	* expand/rust-expand-visitor.h: Likewise.
	* hir/rust-ast-lower-base.cc (ASTLoweringBase::lower_binding):
	Likewise.
	(ASTLoweringBase::lower_generic_args): Likewise.
	(ASTLoweringBase::lower_self): Likewise.
	(ASTLoweringBase::lower_type_no_bounds): Likewise.
	(ASTLoweringBase::lower_bound): Likewise.
	(ASTLoweringBase::lower_range_pattern_bound): Likewise.
	* hir/rust-ast-lower-base.h: Likewise.
	* hir/rust-ast-lower-block.h: Likewise.
	* hir/rust-ast-lower-enumitem.h: Likewise.
	* hir/rust-ast-lower-expr.cc (ASTLoweringExpr::translate): Likewise.
	(ASTLoweringExpr::visit): Likewise.
	* hir/rust-ast-lower-expr.h: Likewise.
	* hir/rust-ast-lower-extern.h: Likewise.
	* hir/rust-ast-lower-implitem.cc (ASTLowerImplItem::translate):
	Likewise.
	(ASTLowerImplItem::visit): Likewise.
	(ASTLowerTraitItem::translate): Likewise.
	(ASTLowerTraitItem::visit): Likewise.
	* hir/rust-ast-lower-implitem.h: Likewise.
	* hir/rust-ast-lower-item.cc (ASTLoweringItem::translate): Likewise.
	(ASTLoweringItem::visit): Likewise.
	* hir/rust-ast-lower-item.h: Likewise.
	* hir/rust-ast-lower-pattern.cc (ASTLoweringPattern::translate):
	Likewise.
	(ASTLoweringPattern::visit): Likewise.
	* hir/rust-ast-lower-pattern.h: Likewise.
	* hir/rust-ast-lower-stmt.cc (ASTLoweringStmt::visit): Likewise.
	* hir/rust-ast-lower-struct-field-expr.h: Likewise.
	* hir/rust-ast-lower-type.cc (ASTLowerTypePath::visit): Likewise.
	(ASTLowerQualifiedPathInType::visit): Likewise.
	(ASTLoweringType::translate): Likewise.
	(ASTLoweringType::visit): Likewise.
	(ASTLowerGenericParam::translate): Likewise.
	(ASTLowerGenericParam::visit): Likewise.
	(ASTLoweringTypeBounds::translate): Likewise.
	(ASTLoweringTypeBounds::visit): Likewise.
	(ASTLowerWhereClauseItem::visit): Likewise.
	* hir/rust-ast-lower-type.h: Likewise.
	* hir/rust-ast-lower.cc (ASTLowering::go): Likewise.
	(ASTLoweringBlock::visit): Likewise.
	(ASTLoweringIfBlock::visit): Likewise.
	(ASTLoweringIfLetBlock::visit): Likewise.
	(ASTLowerStructExprField::visit): Likewise.
	(ASTLoweringExprWithBlock::visit): Likewise.
	(ASTLoweringBase::lower_qual_path_type): Likewise.
	(ASTLoweringBase::lower_closure_param): Likewise.
	* resolve/rust-ast-resolve-base.cc (ResolverBase::resolve_visibility):
	Likewise.
	* resolve/rust-ast-resolve-expr.cc (ResolveExpr::go): Likewise.
	(ResolveExpr::visit): Likewise.
	(ResolveExpr::resolve_closure_param): Likewise.
	* resolve/rust-ast-resolve-expr.h: Likewise.
	* resolve/rust-ast-resolve-implitem.h: Likewise.
	* resolve/rust-ast-resolve-item.cc (ResolveTraitItems::visit):
	Likewise.
	(ResolveItem::go): Likewise.
	(ResolveItem::visit): Likewise.
	(ResolveItem::resolve_impl_item): Likewise.
	(ResolveItem::resolve_extern_item): Likewise.
	(ResolveImplItems::go): Likewise.
	(ResolveExternItem::go): Likewise.
	(ResolveExternItem::visit): Likewise.
	* resolve/rust-ast-resolve-item.h: Likewise.
	* resolve/rust-ast-resolve-path.cc (ResolvePath::go): Likewise.
	(ResolvePath::resolve_path): Likewise.
	* resolve/rust-ast-resolve-path.h: Likewise.
	* resolve/rust-ast-resolve-pattern.cc (PatternDeclaration::go):
	Likewise.
	(PatternDeclaration::visit): Likewise.
	(resolve_range_pattern_bound): Likewise.
	* resolve/rust-ast-resolve-pattern.h: Likewise.
	* resolve/rust-ast-resolve-stmt.cc (ResolveStmt::visit): Likewise.
	* resolve/rust-ast-resolve-stmt.h: Likewise.
	* resolve/rust-ast-resolve-struct-expr-field.cc (ResolveStructExprField::go):
	Likewise.
	(ResolveStructExprField::visit): Likewise.
	* resolve/rust-ast-resolve-struct-expr-field.h: Likewise.
	* resolve/rust-ast-resolve-toplevel.h: Likewise.
	* resolve/rust-ast-resolve-type.cc (ResolveType::visit): Likewise.
	(ResolveRelativeTypePath::go): Likewise.
	(ResolveRelativeQualTypePath::resolve_qual_seg): Likewise.
	(ResolveTypeToCanonicalPath::go): Likewise.
	(ResolveTypeToCanonicalPath::visit): Likewise.
	(ResolveGenericArgs::resolve_disambiguated_generic): Likewise.
	(ResolveGenericArgs::go): Likewise.
	* resolve/rust-ast-resolve-type.h: Likewise.
	* resolve/rust-ast-resolve.cc (NameResolution::go): Likewise.
	* resolve/rust-default-resolver.cc (DefaultResolver::visit): Likewise.
	* resolve/rust-early-name-resolver.cc (EarlyNameResolver::resolve_qualified_path_type):
	Likewise.
	(EarlyNameResolver::visit): Likewise.
	* resolve/rust-toplevel-name-resolver-2.0.cc (TopLevel::visit):
	Likewise.
	* checks/errors/rust-ast-validation.cc: Likewise.

Signed-off-by: Pierre-Emmanuel Patry <pierre-emmanuel.patry@embecosm.com>
@P-E-P P-E-P force-pushed the no-more-ref-to-sp branch from 137f8c1 to e8cc906 Compare March 11, 2024 12:37
@P-E-P P-E-P added this pull request to the merge queue Mar 13, 2024
Merged via the queue into Rust-GCC:master with commit 75dc005 Mar 13, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@powerboat9 powerboat9 powerboat9 left review comments

@CohenArthur CohenArthur CohenArthur approved these changes

Assignees

@P-E-P P-E-P

Labels
cleanup
Projects
Project Pineapple
Status: Done
Milestone
GCC 14.1 release
Development

Successfully merging this pull request may close these issues.
3 participants
@P-E-P @powerboat9 @CohenArthur