removed EnumItemDiscriminant and associated functions #3355

saeitoshi-10 · 2025-01-06T20:40:49Z

* ast/rust-ast-collector.cc (TokenCollector::visit):removed fn
* ast/rust-ast-collector.h: "removed defn"
* ast/rust-ast-full-decls.h (class EnumItemDiscriminant):remove fn
* ast/rust-ast-visitor.cc (DefaultASTVisitor::visit):removed fn
* ast/rust-ast-visitor.h:"removed defn"
* ast/rust-ast.cc (EnumItemDiscriminant::as_string):removed fn (EnumItemDiscriminant::accept_vis):removed fn (FormatArgs::set_outer_attrs): removed fn
* ast/rust-item.h (class EnumItem): "added Discriminant expr" (class EnumItemDiscriminant): "removed redundant class"
* expand/rust-cfg-strip.cc (CfgStrip::visit):removed fn
* expand/rust-cfg-strip.h:"removed defn"
* expand/rust-derive.h:"removed defn"
* expand/rust-expand-visitor.cc (ExpandVisitor::visit):removed fn
* expand/rust-expand-visitor.h:"removed defn"
* hir/rust-ast-lower-base.cc (ASTLoweringBase::visit):removed fn
* hir/rust-ast-lower-base.h:removed defn
* hir/rust-ast-lower-enumitem.h:removed defn
* parse/rust-parse-impl.h (Parser::parse_enum_item):"modified fn" (Parser::parse_pattern): modified fn
* resolve/rust-ast-resolve-base.cc (ResolverBase::visit):removed fn
* resolve/rust-ast-resolve-base.h:"removed defn"
* resolve/rust-ast-resolve-item.cc (ResolveItem::visit):removed fn
* resolve/rust-ast-resolve-item.h:"removed defn"
* resolve/rust-ast-resolve-stmt.h:"removed defn"
* resolve/rust-ast-resolve-toplevel.h:"removed defn"
* resolve/rust-toplevel-name-resolver-2.0.cc (TopLevel::visit): "fn"
* resolve/rust-toplevel-name-resolver-2.0.h:"removed defn"
* util/rust-attributes.cc (AttributeChecker::visit):removed fn
* util/rust-attributes.h:"removed defn"

Thank you for making Rust GCC better!

If your PR fixes an issue, you can add "Fixes #issue_number" into this
PR description and the git commit message. This way the issue will be
automatically closed when your PR is merged. If your change addresses
an issue but does not fully fix it please mark it as "Addresses #issue_number"
in the git commit message.

Here is a checklist to help you with your PR.

GCC development requires copyright assignment or the Developer's Certificate of Origin sign-off, see https://gcc.gnu.org/contribute.html or https://gcc.gnu.org/dco.html
Read contributing guidlines
make check-rust passes locally
Run clang-format
Added any relevant test cases to gcc/testsuite/rust/

Note that you can skip the above if you are just opening a WIP PR in
order to get feedback.

Fixes #3340

*Please write a comment explaining your change. This is the message
that will be part of the merge commit.

* ast/rust-ast-collector.cc (TokenCollector::visit):removed fn * ast/rust-ast-collector.h: "removed defn" * ast/rust-ast-full-decls.h (class EnumItemDiscriminant):remove fn * ast/rust-ast-visitor.cc (DefaultASTVisitor::visit):removed fn * ast/rust-ast-visitor.h:"removed defn" * ast/rust-ast.cc (EnumItemDiscriminant::as_string):removed fn (EnumItemDiscriminant::accept_vis):removed fn (FormatArgs::set_outer_attrs): removed fn * ast/rust-item.h (class EnumItem): "added Discriminant expr" (class EnumItemDiscriminant): "removed redundant class" * expand/rust-cfg-strip.cc (CfgStrip::visit):removed fn * expand/rust-cfg-strip.h:"removed defn" * expand/rust-derive.h:"removed defn" * expand/rust-expand-visitor.cc (ExpandVisitor::visit):removed fn * expand/rust-expand-visitor.h:"removed defn" * hir/rust-ast-lower-base.cc (ASTLoweringBase::visit):removed fn * hir/rust-ast-lower-base.h:removed defn * hir/rust-ast-lower-enumitem.h:removed defn * parse/rust-parse-impl.h (Parser::parse_enum_item):"modified fn" (Parser::parse_pattern): modified fn * resolve/rust-ast-resolve-base.cc (ResolverBase::visit):removed fn * resolve/rust-ast-resolve-base.h:"removed defn" * resolve/rust-ast-resolve-item.cc (ResolveItem::visit):removed fn * resolve/rust-ast-resolve-item.h:"removed defn" * resolve/rust-ast-resolve-stmt.h:"removed defn" * resolve/rust-ast-resolve-toplevel.h:"removed defn" * resolve/rust-toplevel-name-resolver-2.0.cc (TopLevel::visit): "fn" * resolve/rust-toplevel-name-resolver-2.0.h:"removed defn" * util/rust-attributes.cc (AttributeChecker::visit):removed fn * util/rust-attributes.h:"removed defn" Signed-off-by: Om Swaroop Nayak <[email protected]>

dkm · 2025-01-07T12:45:06Z

Hi!
Would it be possible to cleanup the changelog part a bit?
You don't need to quote the description with ".
The idiomatic way of removing something is:

    * file.c (function_name): Removed.

Things like:

* resolve/rust-ast-resolve-item.h:"removed defn"

are note very helpful... Maybe something like:

* resolve/rust-ast-resolve-item.h (visit (AST::EnumItemDiscriminant &item)): Removed.

would be a bit more descriptive.

Also, having an actual description of the change in the commit log would be appreciated. Don't forget that when the changes are contributed to GCC, there's no link to this pull request, and it's expected that the commit log has all the information.

Thanks!

saeitoshi-10 · 2025-01-07T12:53:37Z

Hi @dkm !
I’ll work on cleaning up the changelog to follow the standard format and make the entries more descriptive. I’ll also ensure the commit log includes a proper description of the changes for clarity when they are contributed to GCC. But if i add a lengthy description then the changelog test fails for exceeding length limit. what should i do ?

* ast/rust-ast-collector.cc (TokenCollector::visit):Removed * ast/rust-ast-collector.h: Removed TokenCollector::visit definition * ast/rust-ast-full-decls.h (class EnumItemDiscriminant):Removed * ast/rust-ast-visitor.cc (DefaultASTVisitor::visit):Removed * ast/rust-ast-visitor.h:Removed DefaultASTVisitor::visit definition * ast/rust-ast.cc (EnumItemDiscriminant::as_string):Removed (EnumItemDiscriminant::accept_vis):Removed (FormatArgs::set_outer_attrs): Removed * ast/rust-item.h (class EnumItem): added a discriminant expression attribute with relevant helper functions and a constructor to initialise enum items with discriminant (class EnumItemDiscriminant): Removed * expand/rust-cfg-strip.cc (CfgStrip::visit):Removed * expand/rust-cfg-strip.h:Removed CfgStrip::visit definition * expand/rust-derive.h:Removed CfgStrip::visit definition * expand/rust-expand-visitor.cc (ExpandVisitor::visit):Removed * expand/rust-expand-visitor.h:Removed ExpandVisitor::visit definition * hir/rust-ast-lower-base.cc (ASTLoweringBase::visit):Removed * hir/rust-ast-lower-base.h:Removed ASTLoweringBase::visit definition * hir/rust-ast-lower-enumitem.h:Removed visit function * parse/rust-parse-impl.h (Parser::parse_enum_item): removed the EnumItemDiscriminant constructor and added the EnumItem constructor with expression (Parser::parse_pattern): modified * resolve/rust-ast-resolve-base.cc (ResolverBase::visit):Removed * resolve/rust-ast-resolve-base.h:Removed ResolverBase::visit definition * resolve/rust-ast-resolve-item.cc (ResolveItem::visit):Removed * resolve/rust-ast-resolve-item.h:Removed ResolveItem::visit definition * resolve/rust-ast-resolve-stmt.h:Removed visit function * resolve/rust-ast-resolve-toplevel.h:Removed visit function * resolve/rust-toplevel-name-resolver-2.0.cc (TopLevel::visit):Removed * resolve/rust-toplevel-name-resolver-2.0.h:Removed TopLevel::visit definition * util/rust-attributes.cc (AttributeChecker::visit):Removed * util/rust-attributes.h:Removed AttributeChecker::visit definition Signed-off-by: Om Swaroop Nayak <[email protected]>

…dis_rm

saeitoshi-10 · 2025-01-07T15:58:05Z

hi @dkm is the new changelog fine

CohenArthur · 2025-01-08T13:13:14Z

@saeitoshi-10 the changelog checks are failing, as you said because the lines are too long. You can just put the second half on a second line with a leading tab character, like in this commit de8606f

CohenArthur · 2025-01-08T13:21:55Z

gcc/rust/parse/rust-parse-impl.h

-					 std::move (discriminant_expr),
-					 std::move (outer_attrs),
-					 item_name_tok->get_locus ()));
+	return std::unique_ptr<AST::EnumItem> (new AST::EnumItem (


here we need to add a comment saying that this part needs to be reworked to properly handle enum discriminants, and link to issue #3340

something like:

// FIXME: We need to add special handling for parsing discriminant expressions for all possible enum items, not just enum item identifiers (gccrs#3340)

CohenArthur · 2025-01-08T13:23:37Z

gcc/rust/parse/rust-parse-impl.h

-					 item_name_tok->get_locus ()));
+	return std::unique_ptr<AST::EnumItem> (new AST::EnumItem (
+	  std::move (item_name), std::move (vis), std::move (outer_attrs),
+	  item_name_tok->get_locus (), discriminant_expr->clone_expr ()));


We need to not use the discriminant_expr here as we can't do anything with it yet. So use the regular constructor.

Suggested change

item_name_tok->get_locus (), discriminant_expr->clone_expr ()));

item_name_tok->get_locus ()));

CohenArthur · 2025-01-08T13:24:34Z

gcc/rust/ast/rust-item.h

+  std::unique_ptr<Expr> expression;
+


I don't think we want to add handling for this just yet because we're not handling it properly and parsing it for all enum items yet. So I would say remove this

CohenArthur · 2025-01-08T13:24:46Z

gcc/rust/ast/rust-item.h

+  EnumItem (Identifier variant_name, Visibility vis,
+	    std::vector<Attribute> outer_attrs, location_t locus,
+	    std::unique_ptr<AST::Expr> Discriminant)
+    : VisItem (std::move (vis), std::move (outer_attrs)),
+      variant_name (std::move (variant_name)), locus (locus),
+      expression (Discriminant->clone_expr ())
+  {}
+
+  EnumItem (EnumItem const &other)
+    : VisItem (other), variant_name (other.variant_name), locus (other.locus),
+      expression (other.expression->clone_expr ())
+  {}
+


remove these constructors

CohenArthur · 2025-01-08T13:24:58Z

gcc/rust/ast/rust-item.h

+  bool has_expr () { return expression != nullptr; }
+
+  Expr &get_expr ()
+  {
+    rust_assert (expression != nullptr);
+    return *expression;
+  }
+
+  std::unique_ptr<Expr> &get_expr_ptr ()
+  {
+    rust_assert (expression != nullptr);
+    return expression;
+  }
+


we don't need these as well

saeitoshi-10 changed the title ~~gcc/rust/ChangeLog:~~ removed EnumItemDiscriminant and associated functions Jan 7, 2025

saeitoshi-10 added 2 commits January 7, 2025 06:18

Merge branch 'enum_dis_rm' of github.com:itoshi69sae/gccrs into enum_…

db53b76

…dis_rm

saeitoshi-10 added 2 commits January 7, 2025 22:17

Update rust-ast.cc

ea9ed37

Update rust-parse-impl.h

057c7e6

CohenArthur requested changes Jan 8, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

removed EnumItemDiscriminant and associated functions #3355

removed EnumItemDiscriminant and associated functions #3355

saeitoshi-10 commented Jan 6, 2025

dkm commented Jan 7, 2025

saeitoshi-10 commented Jan 7, 2025 •

edited

Loading

saeitoshi-10 commented Jan 7, 2025

CohenArthur commented Jan 8, 2025

CohenArthur Jan 8, 2025

CohenArthur Jan 8, 2025

CohenArthur Jan 8, 2025

CohenArthur Jan 8, 2025

CohenArthur Jan 8, 2025

	item_name_tok->get_locus (), discriminant_expr->clone_expr ()));
	item_name_tok->get_locus ()));

removed EnumItemDiscriminant and associated functions #3355

Are you sure you want to change the base?

removed EnumItemDiscriminant and associated functions #3355

Conversation

saeitoshi-10 commented Jan 6, 2025

Note that you can skip the above if you are just opening a WIP PR in order to get feedback.

dkm commented Jan 7, 2025

saeitoshi-10 commented Jan 7, 2025 • edited Loading

saeitoshi-10 commented Jan 7, 2025

CohenArthur commented Jan 8, 2025

CohenArthur Jan 8, 2025

Choose a reason for hiding this comment

CohenArthur Jan 8, 2025

Choose a reason for hiding this comment

CohenArthur Jan 8, 2025

Choose a reason for hiding this comment

CohenArthur Jan 8, 2025

Choose a reason for hiding this comment

CohenArthur Jan 8, 2025

Choose a reason for hiding this comment

Note that you can skip the above if you are just opening a WIP PR in
order to get feedback.

saeitoshi-10 commented Jan 7, 2025 •

edited

Loading