Add dump option to show node #3212

Kamiinarii78 · 2024-10-22T14:03:05Z

Add a new dump option to show node node as
internal comment with a blacklist to ignore some
node.

gcc/rust/ChangeLog:

* ast/rust-ast-collector.cc (TokenCollector::begin_internal_comment): Add internal comment to print node beginning. (TokenCollector::end_internal_comment): Add internal comment to print node end. (TokenCollector::visit): Add the comments of the node visited. (TokenCollector::visit_closure_common): Likewise. (TokenCollector::visit_loop_common): Likewise.
* ast/rust-ast-collector.h: Add internal comment as a nes Kind.
* ast/rust-ast-dump.cc (Dump::Dump): add a Dump constructor to enable internal.
* ast/rust-ast-dump.h: Add printing of internal comment in the dump
* rust-session-manager.cc (Session::enable_dump): Activate ast dump and fill the blacklist. (Session::handle_internal_blacklist): Parse the flag to get node to be blacklisted. (Session::compile_crate): Launch ast dump internal when asked. (Session::dump_ast_pretty_internal): Call the visitor to dump the internals.
* rust-session-manager.h (struct CompileOptions): add Interal in Dump option enum.

P-E-P · 2024-10-23T10:43:03Z

gcc/rust/ast/rust-ast-collector.h

@@ -23,6 +23,7 @@
 #include "rust-ast-visitor.h"
 #include "rust-ast.h"
 #include "rust-ast-full.h"
+#include <sstream>


You probably want to include "rust-system.h" instead.

CohenArthur

This is great work!! I've added a couple comments but they're mostly nitpick. The feature in itself is solid and you did good :) Thanks!!!!

CohenArthur · 2024-10-30T13:45:14Z

gcc/rust/ast/rust-ast-collector.cc

+  begin_internal_comment ("VariadicParam");
+
  if (param.has_pattern ())
    {
      visit (param.get_pattern ());
      push (Rust::Token::make (COLON, UNDEF_LOCATION));
    }
  push (Rust::Token::make (ELLIPSIS, UNDEF_LOCATION));
+
+  end_internal_comment ("VariadicParam");


This pattern is nice but if forces you to write the node's "name" twice - once when you begin the comment, and once when you end it. It would be nicer and easier to maintain if this was wrapped in a lambda of some sort, e.g.:

internal_comment("VariadicParam", []() { if (param.has_pattern ()) { visit (param.get_pattern ()); push (Rust::Token::make (COLON, UNDEF_LOCATION)); } push (Rust::Token::make (ELLIPSIS, UNDEF_LOCATION)); });

and the code for internal_comment could be something like the following:

template <typename T> internal_comment(std::string node_name, std::function<void(T& node)> visitor) { begin_internal_comment(node_name); visitor(); end_internal_comment(node_name); }

note that the ergonomics might not be amazing, in which case we can look at something else like a custom destructor like what @jdupak did recently (we talked about it a little bit on Zulip)

CohenArthur · 2024-10-30T13:49:30Z

gcc/rust/ast/rust-ast-collector.h

    Newline,
    Indentation,
    Token,
  };

  CollectItem (TokenPtr token) : token (token), kind (Kind::Token) {}
-  CollectItem (std::string comment) : comment (comment), kind (Kind::Comment) {}
+  CollectItem (std::string comment, bool internal = false)


I would avoid using the boolean here and expose a new enum instead, which would make the code clearer

enum class Comment { Regular, Internal, }; CollectItem (std::string comment, Comment kind = Comment::Regular) : comment(comment), kind(kind == Comment::Internal ? Kind::InternalComment : Kind::Comment) { ... }

CohenArthur · 2024-10-30T13:52:33Z

gcc/rust/ast/rust-ast-dump.h

@@ -32,6 +32,8 @@ class Dump
 {
 public:
  Dump (std::ostream &stream);
+  Dump (std::ostream &stream, bool print_internal,
+	std::vector<std::string> blacklist);


we can use a std::set<std::string> here for faster lookup, and for slightly better APIs as well since we can do blacklist.contains(...). also, I would call the set forbidden or excluded instead of blacklist

CohenArthur · 2024-10-30T13:53:12Z

gcc/rust/rust-session-manager.cc

+#include <sstream>
+#include <vector>



These includes are prohibited here for obscure bootstrapping compiler reasons, but you can include rust-system.h instead

CohenArthur · 2024-10-30T13:54:20Z

gcc/rust/rust-session-manager.cc

+void
+Session::handle_internal_blacklist (std::string arg)
+{
+  std::istringstream blist_str (arg.substr (arg.find (":") + 1, 50));


what is 50 here? can we use a define or constant instead?

CohenArthur · 2024-10-30T13:55:00Z

gcc/rust/rust-session-manager.cc

+
+  std::vector<std::string> str_tmp = options.get_blacklist ();
+
+  AST::Dump (out, true, str_tmp).go (crate);


same here, ideally we'd use something other than true like an enum

CohenArthur · 2024-10-30T13:55:21Z

gcc/rust/rust-session-manager.h

  };

  std::set<DumpOption> dump_options;

+  /* List of node that is not print during the dump of the ast with internal
+   * comment */
+  std::vector<std::string> internal_blacklist;


Suggested change

std::vector<std::string> internal_blacklist;

std::set<std::string> excluded_nodes;

Add a new dump option to show node node as internal comment with a blacklist to ignore some node. gcc/rust/ChangeLog: * ast/rust-ast-collector.cc (TokenCollector::begin_internal_comment): Add internal comment to print node beginning. (TokenCollector::end_internal_comment): Add internal comment to print node end. (TokenCollector::visit): Add the comments of the node visited. (TokenCollector::visit_closure_common): Likewise. (TokenCollector::visit_loop_common): Likewise. * ast/rust-ast-collector.h: Add internal comment as a nes Kind. * ast/rust-ast-dump.cc (Dump::Dump): add a Dump constructor to enable internal. * ast/rust-ast-dump.h: Add printing of internal comment in the dump * rust-session-manager.cc (Session::enable_dump): Activate ast dump and fill the blacklist. (Session::handle_internal_blacklist): Parse the flag to get node to be blacklisted. (Session::compile_crate): Launch ast dump internal when asked. (Session::dump_ast_pretty_internal): Call the visitor to dump the internals. * rust-session-manager.h (struct CompileOptions): add Interal in Dump option enum. Signed-off-by: Benjamin Thos <benjamin.thos@epita.fr>

Use an enum instead of bool to check if a comment is internal, use good include and replace some variable name. gcc/rust/ChangeLog: * ast/rust-ast-collector.cc (TokenCollector::begin_internal_comment): Change a boolean with an enum. (TokenCollector::end_internal_comment): Likewise. * ast/rust-ast-collector.h: Likewise + change include. * ast/rust-ast-dump.cc (Dump::Dump): Change variable name. * ast/rust-ast-dump.h: Likewise + replace vector with a set. * rust-session-manager.cc (Session::enable_dump): Change variable name. (Session::handle_internal_blacklist): Change function name. (Session::handle_excluded_node): Likewise. (Session::dump_ast_pretty_internal): Change vector with a set. * rust-session-manager.h (struct CompileOptions): Likewise + change variable name. Signed-off-by: Benjamin Thos <benjamin.thos@epita.fr>

When we want to add an internal comment we know call one function that wrap the code instead of calling two function, one at the beginning one at the end. gcc/rust/ChangeLog: * ast/rust-ast-collector.cc (TokenCollector::internal_comment): Wrapper function to add both comment. (TokenCollector::visit): call of the wrapper function. (TokenCollector::visit_closure_common): Same. (TokenCollector::visit_loop_common): Same. * ast/rust-ast-collector.h: Prototype of the wrapper function Signed-off-by: Benjamin Thos <benjamin.thos@epita.fr>

philberty

some changes still needed

philberty · 2025-01-10T12:49:34Z

gcc/rust/rust-session-manager.cc

@@ -1,4 +1,4 @@
-// Copyright (C) 2020-2024 Free Software Foundation, Inc.
+// Copyright (C) 2020 - 2024 Free Software Foundation, Inc.


dont change this. This gets auto updated with a script

philberty · 2025-01-10T12:49:50Z

gcc/rust/rust-session-manager.cc

+  else if (!arg.compare (0, 8, "internal"))
+    {
+      if (arg.size () == 8)
+	{


no need for 2nd level of braces

philberty · 2025-01-10T12:50:24Z

gcc/rust/rust-session-manager.cc

+	}
+      else
+	{
+	  if (arg[8] != ':')


that could just be an else if

philberty · 2025-01-10T12:51:10Z

gcc/rust/rust-session-manager.cc

@@ -387,6 +411,22 @@ Session::enable_dump (std::string arg)
  return true;
 }

+/* Helper function to parse a string when dump internal to get node to blacklist


do // style comments please

P-E-P requested a review from CohenArthur October 22, 2024 14:05

Kamiinarii78 force-pushed the intern-comment-impl branch from 640795a to 9e874a5 Compare October 22, 2024 14:24

P-E-P reviewed Oct 23, 2024

View reviewed changes

CohenArthur reviewed Oct 30, 2024

View reviewed changes

CohenArthur marked this pull request as draft October 31, 2024 13:44

Kamiinarii78 force-pushed the intern-comment-impl branch from c4e2e4d to 1688286 Compare December 13, 2024 15:17

Kamiinarii78 marked this pull request as ready for review December 16, 2024 09:34

Kamiinarii added 3 commits December 16, 2024 11:22

Kamiinarii78 force-pushed the intern-comment-impl branch from eaac4f2 to 01ae1b5 Compare December 16, 2024 10:23

philberty requested changes Jan 10, 2025

View reviewed changes

philberty assigned Kamiinarii78 Jan 10, 2025

philberty added AST debug labels Jan 10, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add dump option to show node #3212

Add dump option to show node #3212

Kamiinarii78 commented Oct 22, 2024

P-E-P Oct 23, 2024

CohenArthur left a comment

CohenArthur Oct 30, 2024

CohenArthur Oct 30, 2024

CohenArthur Oct 30, 2024

CohenArthur Oct 30, 2024

CohenArthur Oct 30, 2024

CohenArthur Oct 30, 2024

CohenArthur Oct 30, 2024

philberty left a comment

philberty Jan 10, 2025

philberty Jan 10, 2025

philberty Jan 10, 2025

philberty Jan 10, 2025


		std::vector<std::string> str_tmp = options.get_blacklist ();

		AST::Dump (out, true, str_tmp).go (crate);

	std::vector<std::string> internal_blacklist;
	std::set<std::string> excluded_nodes;

		@@ -1,4 +1,4 @@
		// Copyright (C) 2020-2024 Free Software Foundation, Inc.
		// Copyright (C) 2020 - 2024 Free Software Foundation, Inc.

Add dump option to show node #3212

Are you sure you want to change the base?

Add dump option to show node #3212

Conversation

Kamiinarii78 commented Oct 22, 2024

Choose a reason for hiding this comment

CohenArthur left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philberty left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment