diff mbox series

[pushed] diagnostics: consolidate on_{begin, end}_diagnostic into on_report_diagnostic

Message ID 20240826163615.2393695-1-dmalcolm@redhat.com
State New
Headers show
Series [pushed] diagnostics: consolidate on_{begin, end}_diagnostic into on_report_diagnostic | expand

Commit Message

David Malcolm Aug. 26, 2024, 4:36 p.m. UTC
Previously diagnostic_context::report_diagnostic had, after the call to
pp_format (phases 1 and 2 of formatting the message):

  m_output_format->on_begin_diagnostic (*diagnostic);
  pp_output_formatted_text (this->printer, m_urlifier);
  if (m_show_cwe)
    print_any_cwe (*diagnostic);
  if (m_show_rules)
    print_any_rules (*diagnostic);
  if (m_show_option_requested)
  print_option_information (*diagnostic, orig_diag_kind);
  m_output_format->on_end_diagnostic (*diagnostic, orig_diag_kind);

This patch replaces all of the above with a single call to

  m_output_format->on_report_diagnostic (*diagnostic, orig_diag_kind);

moving responsibility for phase 3 of formatting and printing the result
from diagnostic_context to the output format.

This simplifies diagnostic_context::report_diagnostic and allows us to
move the code that prints CWEs, rules, and option information in textual
form from diagnostic_context to diagnostic_text_output_format, where it
belongs.

No functional change intended.

Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu.
Pushed to trunk as r15-3200-gac707d30ce449f.

gcc/ChangeLog:
	* diagnostic-format-json.cc
	(json_output_format::on_begin_diagnostic): Delete.
	(json_output_format::on_end_diagnostic): Rename to...
	(json_output_format::on_report_diagnostic): ...this and add call
	to pp_output_formatted_text.
	(diagnostic_output_format_init_json): Drop unnecessary calls
	to disable textual printing of CWEs, rules, and options.
	* diagnostic-format-sarif.cc (sarif_builder::end_diagnostic):
	Rename to...
	(sarif_builder::on_report_diagnostic): ...this and add call to
	pp_output_formatted_text.
	(sarif_output_format::on_begin_diagnostic): Delete.
	(sarif_output_format::on_end_diagnostic): Rename to...
	(sarif_output_format::on_report_diagnostic): ...this and update
	call to m_builder accordingly.
	(diagnostic_output_format_init_sarif): Drop unnecessary calls
	to disable textual printing of CWEs, rules, and options.
	* diagnostic.cc (diagnostic_context::print_any_cwe): Convert to...
	(diagnostic_text_output_format::print_any_cwe): ...this.
	(diagnostic_context::print_any_rules): Convert to...
	(diagnostic_text_output_format::print_any_rules): ...this.
	(diagnostic_context::print_option_information): Convert to...
	(diagnostic_text_output_format::print_option_information):
	...this.
	(diagnostic_context::report_diagnostic): Replace calls to the
	output format's on_begin_diagnostic, to pp_output_formatted_text,
	printing CWE, rules, option info, and the call to the format's
	on_end_diagnostic with a call to the format's
	on_report_diagnostic.
	(diagnostic_text_output_format::on_begin_diagnostic): Delete.
	(diagnostic_text_output_format::on_end_diagnostic): Delete.
	(diagnostic_text_output_format::on_report_diagnostic): New vfunc,
	which effectively does the on_begin_diagnostic, the call to
	pp_output_formatted_text, the calls for printing CWE, rules,
	option info, and the call to the diagnostic_finalizer.
	* diagnostic.h (diagnostic_output_format::on_begin_diagnostic):
	Delete.
	(diagnostic_output_format::on_end_diagnostic): Delete.
	(diagnostic_output_format::on_report_diagnostic): New.
	(diagnostic_text_output_format::on_begin_diagnostic): Delete.
	(diagnostic_text_output_format::on_end_diagnostic): Delete.
	(diagnostic_text_output_format::on_report_diagnostic): New.
	(class diagnostic_context): Add friend class
	diagnostic_text_output_format.
	(diagnostic_context::get_urlifier): New accessor.
	(diagnostic_context::print_any_cwe): Move decl...
	(diagnostic_text_output_format::print_any_cwe): ...to here.
	(diagnostic_context::print_any_rules): Move decl...
	(diagnostic_text_output_format::print_any_rules): ...to here.
	(diagnostic_context::print_option_information): Move decl...
	(diagnostic_text_output_format::print_option_information): ...to
	here.

Signed-off-by: David Malcolm <dmalcolm@redhat.com>
---
 gcc/diagnostic-format-json.cc  |  24 +--
 gcc/diagnostic-format-sarif.cc |  34 ++---
 gcc/diagnostic.cc              | 261 +++++++++++++++++----------------
 gcc/diagnostic.h               |  28 ++--
 4 files changed, 171 insertions(+), 176 deletions(-)
diff mbox series

Patch

diff --git a/gcc/diagnostic-format-json.cc b/gcc/diagnostic-format-json.cc
index b78cb92cfd7a..f2e9d0d79e51 100644
--- a/gcc/diagnostic-format-json.cc
+++ b/gcc/diagnostic-format-json.cc
@@ -47,13 +47,8 @@  public:
     m_cur_children_array = nullptr;
   }
   void
-  on_begin_diagnostic (const diagnostic_info &) final override
-  {
-    /* No-op.  */
-  }
-  void
-  on_end_diagnostic (const diagnostic_info &diagnostic,
-		     diagnostic_t orig_diag_kind) final override;
+  on_report_diagnostic (const diagnostic_info &diagnostic,
+			diagnostic_t orig_diag_kind) final override;
   void on_diagram (const diagnostic_diagram &) final override
   {
     /* No-op.  */
@@ -225,14 +220,16 @@  make_json_for_path (diagnostic_context &context,
 }
 
 
-/* Implementation of "on_end_diagnostic" vfunc for JSON output.
+/* Implementation of "on_report_diagnostic" vfunc for JSON output.
    Generate a JSON object for DIAGNOSTIC, and store for output
    within current diagnostic group.  */
 
 void
-json_output_format::on_end_diagnostic (const diagnostic_info &diagnostic,
-				       diagnostic_t orig_diag_kind)
+json_output_format::on_report_diagnostic (const diagnostic_info &diagnostic,
+					  diagnostic_t orig_diag_kind)
 {
+  pp_output_formatted_text (m_context.printer, m_context.get_urlifier ());
+
   json::object *diag_obj = new json::object ();
 
   /* Get "kind" of diagnostic.  */
@@ -395,13 +392,6 @@  diagnostic_output_format_init_json (diagnostic_context &context)
   /* Suppress normal textual path output.  */
   context.set_path_format (DPF_NONE);
 
-  /* The metadata is handled in JSON format, rather than as text.  */
-  context.set_show_cwe (false);
-  context.set_show_rules (false);
-
-  /* The option is handled in JSON format, rather than as text.  */
-  context.set_show_option_requested (false);
-
   /* Don't colorize the text.  */
   pp_show_color (context.printer) = false;
   context.set_show_highlight_colors (false);
diff --git a/gcc/diagnostic-format-sarif.cc b/gcc/diagnostic-format-sarif.cc
index 1d99c904ff0c..554bf3cb2d5c 100644
--- a/gcc/diagnostic-format-sarif.cc
+++ b/gcc/diagnostic-format-sarif.cc
@@ -592,9 +592,9 @@  public:
 		 const char *main_input_filename_,
 		 bool formatted);
 
-  void end_diagnostic (diagnostic_context &context,
-		       const diagnostic_info &diagnostic,
-		       diagnostic_t orig_diag_kind);
+  void on_report_diagnostic (diagnostic_context &context,
+			     const diagnostic_info &diagnostic,
+			     diagnostic_t orig_diag_kind);
   void emit_diagram (diagnostic_context &context,
 		     const diagnostic_diagram &diagram);
   void end_group ();
@@ -1350,13 +1350,15 @@  sarif_builder::sarif_builder (diagnostic_context &context,
 			  false);
 }
 
-/* Implementation of "end_diagnostic" for SARIF output.  */
+/* Implementation of "on_report_diagnostic" for SARIF output.  */
 
 void
-sarif_builder::end_diagnostic (diagnostic_context &context,
-			       const diagnostic_info &diagnostic,
-			       diagnostic_t orig_diag_kind)
+sarif_builder::on_report_diagnostic (diagnostic_context &context,
+				     const diagnostic_info &diagnostic,
+				     diagnostic_t orig_diag_kind)
 {
+  pp_output_formatted_text (context.printer, context.get_urlifier ());
+
   if (diagnostic.kind == DK_ICE || diagnostic.kind == DK_ICE_NOBT)
     {
       m_invocation_obj->add_notification_for_ice (context, diagnostic, *this);
@@ -2920,15 +2922,10 @@  public:
     m_builder.end_group ();
   }
   void
-  on_begin_diagnostic (const diagnostic_info &) final override
+  on_report_diagnostic (const diagnostic_info &diagnostic,
+			    diagnostic_t orig_diag_kind) final override
   {
-    /* No-op,  */
-  }
-  void
-  on_end_diagnostic (const diagnostic_info &diagnostic,
-		     diagnostic_t orig_diag_kind) final override
-  {
-    m_builder.end_diagnostic (m_context, diagnostic, orig_diag_kind);
+    m_builder.on_report_diagnostic (m_context, diagnostic, orig_diag_kind);
   }
   void on_diagram (const diagnostic_diagram &diagram) final override
   {
@@ -3022,13 +3019,6 @@  diagnostic_output_format_init_sarif (diagnostic_context &context)
   /* Override callbacks.  */
   context.set_ice_handler_callback (sarif_ice_handler);
 
-  /* The metadata is handled in SARIF format, rather than as text.  */
-  context.set_show_cwe (false);
-  context.set_show_rules (false);
-
-  /* The option is handled in SARIF format, rather than as text.  */
-  context.set_show_option_requested (false);
-
   /* Don't colorize the text.  */
   pp_show_color (context.printer) = false;
   context.set_show_highlight_colors (false);
diff --git a/gcc/diagnostic.cc b/gcc/diagnostic.cc
index 92bd4f808453..497bbe79705c 100644
--- a/gcc/diagnostic.cc
+++ b/gcc/diagnostic.cc
@@ -1232,117 +1232,6 @@  get_cwe_url (int cwe)
   return xasprintf ("https://cwe.mitre.org/data/definitions/%i.html", cwe);
 }
 
-/* If DIAGNOSTIC has a CWE identifier, print it.
-
-   For example, if the diagnostic metadata associates it with CWE-119,
-   " [CWE-119]" will be printed, suitably colorized, and with a URL of a
-   description of the security issue.  */
-
-void
-diagnostic_context::print_any_cwe (const diagnostic_info &diagnostic)
-{
-  if (diagnostic.metadata == NULL)
-    return;
-
-  int cwe = diagnostic.metadata->get_cwe ();
-  if (cwe)
-    {
-      pretty_printer * const pp = this->printer;
-      char *saved_prefix = pp_take_prefix (pp);
-      pp_string (pp, " [");
-      pp_string (pp, colorize_start (pp_show_color (pp),
-				     diagnostic_kind_color[diagnostic.kind]));
-      if (pp->supports_urls_p ())
-	{
-	  char *cwe_url = get_cwe_url (cwe);
-	  pp_begin_url (pp, cwe_url);
-	  free (cwe_url);
-	}
-      pp_printf (pp, "CWE-%i", cwe);
-      pp_set_prefix (pp, saved_prefix);
-      if (pp->supports_urls_p ())
-	pp_end_url (pp);
-      pp_string (pp, colorize_stop (pp_show_color (pp)));
-      pp_character (pp, ']');
-    }
-}
-
-/* If DIAGNOSTIC has any rules associated with it, print them.
-
-   For example, if the diagnostic metadata associates it with a rule
-   named "STR34-C", then " [STR34-C]" will be printed, suitably colorized,
-   with any URL provided by the rule.  */
-
-void
-diagnostic_context::print_any_rules (const diagnostic_info &diagnostic)
-{
-  if (diagnostic.metadata == NULL)
-    return;
-
-  for (unsigned idx = 0; idx < diagnostic.metadata->get_num_rules (); idx++)
-    {
-      const diagnostic_metadata::rule &rule
-	= diagnostic.metadata->get_rule (idx);
-      if (char *desc = rule.make_description ())
-	{
-	  pretty_printer * const pp = this->printer;
-	  char *saved_prefix = pp_take_prefix (pp);
-	  pp_string (pp, " [");
-	  pp_string (pp,
-		     colorize_start (pp_show_color (pp),
-				     diagnostic_kind_color[diagnostic.kind]));
-	  char *url = NULL;
-	  if (pp->supports_urls_p ())
-	    {
-	      url = rule.make_url ();
-	      if (url)
-		pp_begin_url (pp, url);
-	    }
-	  pp_string (pp, desc);
-	  pp_set_prefix (pp, saved_prefix);
-	  if (pp->supports_urls_p ())
-	    if (url)
-	      pp_end_url (pp);
-	  free (url);
-	  pp_string (pp, colorize_stop (pp_show_color (pp)));
-	  pp_character (pp, ']');
-	  free (desc);
-	}
-    }
-}
-
-/* Print any metadata about the option used to control DIAGNOSTIC to CONTEXT's
-   printer, e.g. " [-Werror=uninitialized]".
-   Subroutine of diagnostic_context::report_diagnostic.  */
-
-void
-diagnostic_context::print_option_information (const diagnostic_info &diagnostic,
-					      diagnostic_t orig_diag_kind)
-{
-  if (char *option_text = make_option_name (diagnostic.option_index,
-					    orig_diag_kind, diagnostic.kind))
-    {
-      char *option_url = nullptr;
-      if (this->printer->supports_urls_p ())
-	option_url = make_option_url (diagnostic.option_index);
-      pretty_printer * const pp = this->printer;
-      pp_string (pp, " [");
-      pp_string (pp, colorize_start (pp_show_color (pp),
-				     diagnostic_kind_color[diagnostic.kind]));
-      if (option_url)
-	pp_begin_url (pp, option_url);
-      pp_string (pp, option_text);
-      if (option_url)
-	{
-	  pp_end_url (pp);
-	  free (option_url);
-	}
-      pp_string (pp, colorize_stop (pp_show_color (pp)));
-      pp_character (pp, ']');
-      free (option_text);
-    }
-}
-
 /* Returns whether a DIAGNOSTIC should be printed, and adjusts diagnostic->kind
    as appropriate for #pragma GCC diagnostic and -Werror=foo.  */
 
@@ -1517,15 +1406,10 @@  diagnostic_context::report_diagnostic (diagnostic_info *diagnostic)
   m_diagnostic_groups.m_emission_count++;
 
   pp_format (this->printer, &diagnostic->message, m_urlifier);
-  m_output_format->on_begin_diagnostic (*diagnostic);
-  pp_output_formatted_text (this->printer, m_urlifier);
-  if (m_show_cwe)
-    print_any_cwe (*diagnostic);
-  if (m_show_rules)
-    print_any_rules (*diagnostic);
-  if (m_show_option_requested)
-    print_option_information (*diagnostic, orig_diag_kind);
-  m_output_format->on_end_diagnostic (*diagnostic, orig_diag_kind);
+  /* Call vfunc in the output format.  This is responsible for
+     phase 3 of formatting, and for printing the result.  */
+  m_output_format->on_report_diagnostic (*diagnostic, orig_diag_kind);
+
   switch (m_extra_output_kind)
     {
     default:
@@ -1815,16 +1699,27 @@  diagnostic_text_output_format::~diagnostic_text_output_format ()
     }
 }
 
+/* Implementation of diagnostic_output_format::on_report_diagnostic vfunc
+   for GCC's standard textual output.  */
+
 void
-diagnostic_text_output_format::on_begin_diagnostic (const diagnostic_info &diagnostic)
+diagnostic_text_output_format::
+on_report_diagnostic (const diagnostic_info &diagnostic,
+		      diagnostic_t orig_diag_kind)
 {
   (*diagnostic_starter (&m_context)) (&m_context, &diagnostic);
-}
 
-void
-diagnostic_text_output_format::on_end_diagnostic (const diagnostic_info &diagnostic,
-						  diagnostic_t orig_diag_kind)
-{
+  pp_output_formatted_text (m_context.printer, m_context.get_urlifier ());
+
+  if (m_context.m_show_cwe)
+    print_any_cwe (diagnostic);
+
+  if (m_context.m_show_rules)
+    print_any_rules (diagnostic);
+
+  if (m_context.m_show_option_requested)
+    print_option_information (diagnostic, orig_diag_kind);
+
   (*diagnostic_finalizer (&m_context)) (&m_context, &diagnostic,
 					orig_diag_kind);
 }
@@ -1843,6 +1738,120 @@  diagnostic_text_output_format::on_diagram (const diagnostic_diagram &diagram)
   pp_flush (m_context.printer);
 }
 
+/* If DIAGNOSTIC has a CWE identifier, print it.
+
+   For example, if the diagnostic metadata associates it with CWE-119,
+   " [CWE-119]" will be printed, suitably colorized, and with a URL of a
+   description of the security issue.  */
+
+void
+diagnostic_text_output_format::print_any_cwe (const diagnostic_info &diagnostic)
+{
+  if (diagnostic.metadata == NULL)
+    return;
+
+  int cwe = diagnostic.metadata->get_cwe ();
+  if (cwe)
+    {
+      pretty_printer * const pp = m_context.printer;
+      char *saved_prefix = pp_take_prefix (pp);
+      pp_string (pp, " [");
+      pp_string (pp, colorize_start (pp_show_color (pp),
+				     diagnostic_kind_color[diagnostic.kind]));
+      if (pp->supports_urls_p ())
+	{
+	  char *cwe_url = get_cwe_url (cwe);
+	  pp_begin_url (pp, cwe_url);
+	  free (cwe_url);
+	}
+      pp_printf (pp, "CWE-%i", cwe);
+      pp_set_prefix (pp, saved_prefix);
+      if (pp->supports_urls_p ())
+	pp_end_url (pp);
+      pp_string (pp, colorize_stop (pp_show_color (pp)));
+      pp_character (pp, ']');
+    }
+}
+
+/* If DIAGNOSTIC has any rules associated with it, print them.
+
+   For example, if the diagnostic metadata associates it with a rule
+   named "STR34-C", then " [STR34-C]" will be printed, suitably colorized,
+   with any URL provided by the rule.  */
+
+void
+diagnostic_text_output_format::
+print_any_rules (const diagnostic_info &diagnostic)
+{
+  if (diagnostic.metadata == NULL)
+    return;
+
+  for (unsigned idx = 0; idx < diagnostic.metadata->get_num_rules (); idx++)
+    {
+      const diagnostic_metadata::rule &rule
+	= diagnostic.metadata->get_rule (idx);
+      if (char *desc = rule.make_description ())
+	{
+	  pretty_printer * const pp = m_context.printer;
+	  char *saved_prefix = pp_take_prefix (pp);
+	  pp_string (pp, " [");
+	  pp_string (pp,
+		     colorize_start (pp_show_color (pp),
+				     diagnostic_kind_color[diagnostic.kind]));
+	  char *url = NULL;
+	  if (pp->supports_urls_p ())
+	    {
+	      url = rule.make_url ();
+	      if (url)
+		pp_begin_url (pp, url);
+	    }
+	  pp_string (pp, desc);
+	  pp_set_prefix (pp, saved_prefix);
+	  if (pp->supports_urls_p ())
+	    if (url)
+	      pp_end_url (pp);
+	  free (url);
+	  pp_string (pp, colorize_stop (pp_show_color (pp)));
+	  pp_character (pp, ']');
+	  free (desc);
+	}
+    }
+}
+
+/* Print any metadata about the option used to control DIAGNOSTIC to CONTEXT's
+   printer, e.g. " [-Werror=uninitialized]".
+   Subroutine of diagnostic_context::report_diagnostic.  */
+
+void
+diagnostic_text_output_format::
+print_option_information (const diagnostic_info &diagnostic,
+			  diagnostic_t orig_diag_kind)
+{
+  if (char *option_text
+      = m_context.make_option_name (diagnostic.option_index,
+				    orig_diag_kind, diagnostic.kind))
+    {
+      char *option_url = nullptr;
+      pretty_printer * const pp = m_context.printer;
+      if (pp->supports_urls_p ())
+	option_url = m_context.make_option_url (diagnostic.option_index);
+      pp_string (pp, " [");
+      pp_string (pp, colorize_start (pp_show_color (pp),
+				     diagnostic_kind_color[diagnostic.kind]));
+      if (option_url)
+	pp_begin_url (pp, option_url);
+      pp_string (pp, option_text);
+      if (option_url)
+	{
+	  pp_end_url (pp);
+	  free (option_url);
+	}
+      pp_string (pp, colorize_stop (pp_show_color (pp)));
+      pp_character (pp, ']');
+      free (option_text);
+    }
+}
+
 /* Set the output format for CONTEXT to FORMAT, using BASE_FILE_NAME for
    file-based output formats.  */
 
diff --git a/gcc/diagnostic.h b/gcc/diagnostic.h
index 2a9f2751dca2..0a496e4bfab9 100644
--- a/gcc/diagnostic.h
+++ b/gcc/diagnostic.h
@@ -208,9 +208,12 @@  public:
 
   virtual void on_begin_group () = 0;
   virtual void on_end_group () = 0;
-  virtual void on_begin_diagnostic (const diagnostic_info &) = 0;
-  virtual void on_end_diagnostic (const diagnostic_info &,
-				  diagnostic_t orig_diag_kind) = 0;
+
+  /* Vfunc with responsibility for phase 3 of formatting the message
+     and "printing" the result.  */
+  virtual void on_report_diagnostic (const diagnostic_info &,
+				     diagnostic_t orig_diag_kind) = 0;
+
   virtual void on_diagram (const diagnostic_diagram &diagram) = 0;
   virtual bool machine_readable_stderr_p () const = 0;
 
@@ -237,14 +240,19 @@  public:
   ~diagnostic_text_output_format ();
   void on_begin_group () override {}
   void on_end_group () override {}
-  void on_begin_diagnostic (const diagnostic_info &) override;
-  void on_end_diagnostic (const diagnostic_info &,
-			  diagnostic_t orig_diag_kind) override;
+  void on_report_diagnostic (const diagnostic_info &,
+			     diagnostic_t orig_diag_kind) override;
   void on_diagram (const diagnostic_diagram &diagram) override;
   bool machine_readable_stderr_p () const final override
   {
     return false;
   }
+
+private:
+  void print_any_cwe (const diagnostic_info &diagnostic);
+  void print_any_rules (const diagnostic_info &diagnostic);
+  void print_option_information (const diagnostic_info &diagnostic,
+				 diagnostic_t orig_diag_kind);
 };
 
 /* A stack of sets of classifications: each entry in the stack is
@@ -382,6 +390,8 @@  public:
   friend diagnostic_finalizer_fn &
   diagnostic_finalizer (diagnostic_context *context);
 
+  friend class diagnostic_text_output_format;
+
   typedef void (*ice_handler_callback_t) (diagnostic_context *);
   typedef void (*set_locations_callback_t) (diagnostic_context *,
 					    diagnostic_info *);
@@ -522,6 +532,7 @@  public:
   {
     return m_client_data_hooks;
   }
+  urlifier *get_urlifier () const { return m_urlifier; }
   text_art::theme *get_diagram_theme () const { return m_diagrams.m_theme; }
 
   int converted_column (expanded_location s) const;
@@ -586,11 +597,6 @@  public:
 private:
   bool includes_seen_p (const line_map_ordinary *map);
 
-  void print_any_cwe (const diagnostic_info &diagnostic);
-  void print_any_rules (const diagnostic_info &diagnostic);
-  void print_option_information (const diagnostic_info &diagnostic,
-				 diagnostic_t orig_diag_kind);
-
   void show_any_path (const diagnostic_info &diagnostic);
 
   void error_recursion () ATTRIBUTE_NORETURN;