Rehaul, Apply code review from Arthur

gcc/rust/ChangeLog:

	* backend/rust-compile-asm.cc (CompileAsm::visit):
	Change API, public/private, comments, formatting from code
	review
	(CompileAsm::asm_build_expr): Likewise.
	(CompileAsm::tree_codegen_asm): Likewise.
	* backend/rust-compile-asm.h (class CompileAsm): Likewise.
	* backend/rust-compile-expr.cc (CompileExpr::visit): Likewise.
	* checks/errors/privacy/rust-privacy-reporter.cc (PrivacyReporter::visit): Likewise.
	* expand/rust-macro-builtins-asm.cc (strip_double_quotes): Likewise.
	(parse_options): Likewise.
	(parse_asm_arg): Likewise.
	(expand_inline_asm_strings): Likewise.
	(parse_asm): Likewise.
	* expand/rust-macro-builtins-asm.h (strip_double_quotes): Likewise.
	(expand_inline_asm_strings): Likewise.
	(expand_inline_asm_string): Likewise.
	* hir/tree/rust-hir-expr.h: Likewise.

gcc/testsuite/ChangeLog:

	* rust/compile/inline_asm_typecheck.rs: Change comments

gcc/rust/backend/rust-compile-asm.cc

@@ -7,14 +7,8 @@ namespace Compile {
 CompileAsm::CompileAsm (Context *ctx)
   : HIRCompileBase (ctx), translated (error_mark_node)
 {}
-void
-CompileAsm::visit (HIR::InlineAsm &expr)
-{
-  ctx->add_statement (asm_build_expr (expr));
-}
-
 tree
-CompileAsm::asm_build_expr (HIR::InlineAsm &expr)
+CompileAsm::tree_codegen_asm (HIR::InlineAsm &expr)
 {
   auto asm_expr
     = asm_build_stmt (expr.get_locus (), {asm_construct_string_tree (expr),

gcc/rust/backend/rust-compile-asm.h

@@ -1,4 +1,3 @@
-
 // Copyright (C) 2020-2024 Free Software Foundation, Inc.
 
 // This file is part of GCC.
@@ -26,16 +25,11 @@
 namespace Rust {
 namespace Compile {
 
-class CompileAsm : private HIRCompileBase, protected HIR::HIRExpressionVisitor
+class CompileAsm : private HIRCompileBase
 {
 private:
   tree translated;
 
-public:
-  // WE WILL OPEN THIS UP WHEN WE WANT TO ADD A DEDICATED PASS OF HIR'S ASM
-  // translation.
-  // static tree Compile (HIR::Expr *expr, Context *ctx);
-
   // RELEVANT MEMBER FUNCTIONS
 
   // The limit is 5 because it stands for the 5 things that the C version of
@@ -46,7 +40,6 @@ class CompileAsm : private HIRCompileBase, protected HIR::HIRExpressionVisitor
   // build_asm_expr (location_t loc, tree string, tree outputs, tree inputs,
   //		tree clobbers, tree labels, bool simple, bool is_inline)
   static const int ASM_TREE_ARRAY_LENGTH = 5;
-  tree asm_build_expr (HIR::InlineAsm &);
   tree asm_build_stmt (location_t,
 		       const std::array<tree, ASM_TREE_ARRAY_LENGTH> &);
 
@@ -56,60 +49,14 @@ class CompileAsm : private HIRCompileBase, protected HIR::HIRExpressionVisitor
   tree asm_construct_clobber_tree (HIR::InlineAsm &);
   tree asm_construct_label_tree (HIR::InlineAsm &);
 
-  CompileAsm (Context *ctx);
-
-  void visit (HIR::InlineAsm &) override;
+public:
+  // WE WILL OPEN THIS UP WHEN WE WANT TO ADD A DEDICATED PASS OF HIR'S ASM
+  // translation.
+  // static tree Compile (HIR::Expr *expr, Context *ctx);
 
-  // NON RELEVANT MEMBER FUNCTIONS
+  CompileAsm (Context *ctx);
 
-  void visit (HIR::TupleIndexExpr &) override {}
-  void visit (HIR::TupleExpr &) override {}
-  void visit (HIR::ReturnExpr &) override {}
-  void visit (HIR::CallExpr &) override {}
-  void visit (HIR::MethodCallExpr &) override {}
-  void visit (HIR::LiteralExpr &) override {}
-  void visit (HIR::AssignmentExpr &) override {}
-  void visit (HIR::CompoundAssignmentExpr &) override {}
-  void visit (HIR::ArrayIndexExpr &) override {}
-  void visit (HIR::ArrayExpr &) override {}
-  void visit (HIR::ArithmeticOrLogicalExpr &) override {}
-  void visit (HIR::ComparisonExpr &) override {}
-  void visit (HIR::LazyBooleanExpr &) override {}
-  void visit (HIR::NegationExpr &) override {}
-  void visit (HIR::TypeCastExpr &) override {}
-  void visit (HIR::IfExpr &) override {}
-  void visit (HIR::IfExprConseqElse &) override {}
-  void visit (HIR::BlockExpr &) override {}
-  void visit (HIR::UnsafeBlockExpr &) override {}
-  void visit (HIR::StructExprStruct &struct_) override {}
-  void visit (HIR::StructExprStructFields &struct_) override {}
-  void visit (HIR::GroupedExpr &) override {}
-  void visit (HIR::FieldAccessExpr &) override {}
-  void visit (HIR::QualifiedPathInExpression &) override {}
-  void visit (HIR::PathInExpression &) override {}
-  void visit (HIR::LoopExpr &) override {}
-  void visit (HIR::WhileLoopExpr &) override {}
-  void visit (HIR::BreakExpr &) override {}
-  void visit (HIR::ContinueExpr &) override {}
-  void visit (HIR::BorrowExpr &) override {}
-  void visit (HIR::DereferenceExpr &) override {}
-  void visit (HIR::MatchExpr &) override {}
-  void visit (HIR::RangeFromToExpr &) override {}
-  void visit (HIR::RangeFromExpr &) override {}
-  void visit (HIR::RangeToExpr &) override {}
-  void visit (HIR::RangeFullExpr &) override {}
-  void visit (HIR::RangeFromToInclExpr &) override {}
-  void visit (HIR::ClosureExpr &) override {}
-  void visit (HIR::ErrorPropagationExpr &) override {}
-  void visit (HIR::RangeToInclExpr &) override {}
-  void visit (HIR::WhileLetLoopExpr &) override {}
-  void visit (HIR::IfLetExpr &) override {}
-  void visit (HIR::IfLetExprConseqElse &) override {}
-  void visit (HIR::AwaitExpr &) override {}
-  void visit (HIR::AsyncBlockExpr &) override {}
-  void visit (HIR::StructExprFieldIdentifier &) override {}
-  void visit (HIR::StructExprFieldIdentifierValue &) override {}
-  void visit (HIR::StructExprFieldIndexValue &) override {}
+  tree tree_codegen_asm (HIR::InlineAsm &);
 };
 } // namespace Compile
 } // namespace Rust

gcc/rust/backend/rust-compile-expr.cc

@@ -321,8 +321,8 @@ CompileExpr::visit (HIR::IfExpr &expr)
 void
 CompileExpr::visit (HIR::InlineAsm &expr)
 {
-  CompileAsm a (ctx);
-  a.visit (expr);
+  CompileAsm asm_codegen (ctx);
+  ctx->add_statement (asm_codegen.tree_codegen_asm (expr));
   // translated = build_asm_expr (0, NULL_TREE, NULL_TREE, NULL_TREE, NULL_TREE,
   //		       NULL_TREE, true, true);
   // CompileAsm::asm_build_expr (expr);

gcc/rust/checks/errors/privacy/rust-privacy-reporter.cc

@@ -285,9 +285,7 @@ PrivacyReporter::visit (HIR::TypePathSegmentFunction &)
 
 void
 PrivacyReporter::visit (HIR::InlineAsm &)
-{
-  return;
-}
+{}
 
 void
 PrivacyReporter::visit (HIR::TypePath &path)

gcc/rust/expand/rust-macro-builtins-asm.cc

@@ -39,6 +39,20 @@ std::map<AST::InlineAsmOption, std::string> InlineAsmOptionMap{
 
 std::set<std::string> potentially_nonpromoted_keywords
   = {"in", "out", "lateout", "inout", "inlateout", "const", "sym", "label"};
+
+std::string
+strip_double_quotes (const std::string &str)
+{
+  // Helper function strips the beginning and ending double quotes from a
+  // string.
+  std::string result = str;
+
+  rust_assert (result.size () >= 3);
+  result.erase (0, 1);
+  result.erase (result.size () - 1, 1);
+  return result;
+}
+
 tl::expected<InlineAsmContext, InlineAsmParseError>
 parse_clobber_abi (InlineAsmContext inline_asm_ctx)
 {
@@ -561,9 +575,7 @@ parse_options (InlineAsmContext &inline_asm_ctx)
 
       // Parse comma as optional
       if (parser.skip_token (COMMA))
-	{
-	  continue;
-	}
+	continue;
       else
 	{
 	  rust_unreachable ();
@@ -689,9 +701,7 @@ parse_asm_arg (InlineAsmContext inline_asm_ctx)
 	{
 	  auto expected = parse_clobber_abi (inline_asm_ctx);
 	  if (expected.has_value ())
-	    {
-	      continue;
-	    }
+	    continue;
 	  else if (expected.error () == COMMITTED)
 	    return expected;
 
@@ -703,9 +713,7 @@ parse_asm_arg (InlineAsmContext inline_asm_ctx)
 	{
 	  auto expected = parse_options (inline_asm_ctx);
 	  if (expected.has_value ())
-	    {
-	      continue;
-	    }
+	    continue;
 	  else if (expected.error () == COMMITTED)
 	    return expected;
 
@@ -718,13 +726,9 @@ parse_asm_arg (InlineAsmContext inline_asm_ctx)
 
       auto expected = parse_reg_operand (inline_asm_ctx);
       if (expected.has_value ())
-	{
-	  continue;
-	}
+	continue;
       else if (expected.error () == COMMITTED)
-	{
-	  return expected;
-	}
+	return expected;
 
       // Since parse_reg_operand is the last thing we've considered,
       // The non-committed parse error type means that we have exhausted our
@@ -742,21 +746,8 @@ parse_asm_arg (InlineAsmContext inline_asm_ctx)
   return tl::expected<InlineAsmContext, InlineAsmParseError> (inline_asm_ctx);
 }
 
-std::string
-strip_double_quotes (const std::string &str)
-{
-  // Helper function strips the beginning and ending double quotes from a
-  // string.
-  std::string result = str;
-
-  rust_assert (result.size () >= 3);
-  result.erase (0, 1);
-  result.erase (result.size () - 1, 1);
-  return result;
-}
-
 tl::expected<InlineAsmContext, InlineAsmParseError>
-expand_inline_asm_strings (InlineAsmContext &inline_asm_ctx)
+expand_inline_asm_strings (InlineAsmContext inline_asm_ctx)
 {
   auto &inline_asm = inline_asm_ctx.inline_asm;
 
@@ -846,20 +837,16 @@ parse_asm (location_t invoc_locus, AST::MacroInvocData &invoc,
 
   auto resulting_context = parse_format_strings (inline_asm_ctx)
 			     .and_then (parse_asm_arg)
-			     .and_then (validate);
+			     .and_then (validate)
+			     .and_then (expand_inline_asm_strings);
 
   // TODO: I'm putting the validation here because the rust reference put
   // it here Per Arthur's advice we would actually do the validation in a
   // different stage. and visit on the InlineAsm AST instead of it's
   // context.
-  auto is_valid = (bool) resulting_context;
-  if (is_valid)
-    {
-      expand_inline_asm_strings (*resulting_context);
-    }
-  if (is_valid)
+  if (resulting_context)
     {
-      auto node = inline_asm_ctx.inline_asm.clone_expr_without_block ();
+      auto node = (*resulting_context).inline_asm.clone_expr_without_block ();
 
       std::vector<AST::SingleASTNode> single_vec = {};
 

gcc/rust/expand/rust-macro-builtins-asm.h

@@ -8,9 +8,6 @@
 #include "system.h"
 namespace Rust {
 
-std::string
-strip_double_quotes (const std::string &str);
-
 enum InlineAsmParseError
 {
   // Enum for InlineAsmParseError
@@ -86,12 +83,9 @@ class InlineAsmContext
     this->allow_templates = allow_templates;
   }
 };
-
-tl::expected<InlineAsmContext, InlineAsmParseError>
-expand_inline_asm_strings (InlineAsmContext &inline_asm_ctx);
-
+WARN_UNUSED_RESULT
 tl::expected<InlineAsmContext, InlineAsmParseError>
-expand_inline_asm_string (InlineAsmContext &inline_asm_ctx);
+expand_inline_asm_strings (InlineAsmContext inline_asm_ctx);
 
 // Expected calls
 WARN_UNUSED_RESULT

gcc/rust/hir/tree/rust-hir-expr.h

@@ -4154,8 +4154,8 @@ class InlineAsm : public ExprWithoutBlock
 
   bool is_inline_asm ()
   {
-    // TODO: Check back later to determine how an InlineAsm is inline.
-    return true;
+    // INFO: An inline asm is asm!, which is the opposite of a global_asm()
+    return !this->is_global_asm;
   }
   InlineAsm (location_t locus, bool is_global_asm,
 	     std::vector<AST::InlineAsmTemplatePiece> template_,

gcc/testsuite/rust/compile/inline_asm_typecheck.rs

@@ -12,10 +12,9 @@ fn main() {
         // This demonstrates that asm!'s is inferred with a unit type is parsed correctly.
         let _ = asm!("nop");
 
-        // This errors out per rust spec
-        //      The asm! block never returns, and its return type is defined as ! (never).
-        //      Behavior is undefined if execution falls through past the end of the asm code.
-        //      A noreturn asm block behaves just like a function which doesn't return; notably, local variables in scope are not dropped before it is invoked.
+        // The asm! block never returns, and its return type is defined as ! (never).
+        // Behavior is undefined if execution falls through past the end of the asm code.
+        // A noreturn asm block behaves just like a function which doesn't return; notably, local variables in scope are not dropped before it is invoked.
         let _ = asm!("nop", options(noreturn));
     }
 }