Hi,
On 9 Jan 2025, at 20:08, Simon Martin wrote:
> On 9 Jan 2025, at 20:00, Marek Polacek wrote:
>
>> On Thu, Jan 09, 2025 at 12:05:43PM -0500, Patrick Palka wrote:
>>> On Wed, 8 Jan 2025, Jason Merrill wrote:
>>>
>>>> On 12/21/24 11:35 AM, Simon Martin wrote:
>>>>> When erroring out due to an incomplete type, we add a contextual
>>>>> note
>>>>> about the type. However, when the error is suppressed by
>>>>> -Wno-template-body, the note remains, making the compiler output
>>>>> quite
>>>>> puzzling.
>>>>>
>>>>> This patch makes sure the note is suppressed if we're processing a
>>>>> template declaration body with -Wno-template-body.
>>>>>
>>>>> Successfully tested on x86_64-pc-linux-gnu.
>>>>>
>>>>> PR c++/118163
>>>>>
>>>>> gcc/cp/ChangeLog:
>>>>>
>>>>> * cp-tree.h (get_current_template): Declare.
>>>>> * error.cc (get_current_template): Make non static.
>>>>> * typeck2.cc (cxx_incomplete_type_inform): Suppress note when
>>>>> parsing a template declaration with -Wno-template-body.
>>>>
>>>> I think rather than adding this sort of thing in lots of places
>>>> where an error
>>>> is followed by an inform, we should change error to return bool
>>>> like
>>>> other
>>>> diagnostic functions, and check its return value before calling
>>>> cxx_incomplete_type_inform or plain inform. This likely involves
>>>> the same
>>>> number of changes, but they should be smaller.
>>>>
>>>> Patrick, what do you think?
>>>
>>> That makes sense to me, it's consistent with the 'warning' API and
>>> how
>>> we handle issuing a warning followed by a note. But since the
>>> -Wtemplate-body mechanism is really only useful for compiling legacy
>>> code where you don't really care about any diagnostics anyway, and
>>> the intended way to use it is -fpermissive /
>>> -Wno-error=template-body
>>> rather than -Wno-template-body, I'd prefer a less invasive solution
>>> that
>>> doesn't change the API of 'error' if possible.
>>>
>>> I wonder if we can work around this by taking advantage of the fact
>>> that
>>> notes that follow an error are expected to be linked via an active
>>> auto_diagnostic_group? Roughly, if we issued a -Wtemplate-body
>>> diagnostic from an active auto_diagnostic_group then all other
>>> diagnostics from that auto_diagnostic_group should also be
>>> associated
>>> with -Wtemplate-body, including notes. That way -Wno-template-body
>>> will
>>> effectively suppress subsequent notes followed by an eligible error,
>>> and
>>> no 'error' callers need to be changed (unless to use
>>> auto_diagnostic_group).
>>
>> FWIW, I love this auto_diagnostic_group idea.
> Thanks folks, I’ll explore the auto_diagnostic_group idea (and maybe
> *also* the error returning bool one because I am not a fan of
> functions
> that “lie” to their callers :-))
>
> I’ll send a follow-up patch in the coming days.
Please find attached an updated version of the patch, that implements
Patrick’s idea and fixes both PR118163 and PR118392. It tracks the
depth at which a warning is inhibited, and suppresses all the notes from
that depth on until an error/warning is emitted or that depth is left.
Successfully tested on x86_64-pc-linux-gnu. OK for GCC 16?
Thanks, Simon
From 865472e427aba34ba07a71e85c8a52619fe381ce Mon Sep 17 00:00:00 2001
From: Simon Martin <si...@nasilyan.com>
Date: Sun, 12 Jan 2025 10:53:00 +0100
Subject: [PATCH] c++: Inhibit subsequent warnings/notes in diagnostic_groups
with an inhibited warning [PR118163,PR118392]
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Those 2 PRs show that even when using a *single* diagnostic_group, it's
possible to end up with a warning being inhibited and its associated
note still emitted, which leads to puzzling user experience. Example
from PR118392:
===
$ gcc/cc1plus inhibit-warn-3.C -w
inhibit-warn-3.C:10:17: note: only here as a âfriendâ
10 | friend void bar();
| ^~~
===
Following a suggestion from ppalka@, this patch keeps track of the
"diagnostic depth" at which a warning in inhibited, and makes sure that
all subsequent notes at that depth or deeper are inhibited as well,
until a subsequent warning or error is accepted at that depth, or the
diagnostic_group or nesting level is popped.
I briefly checked the selftests mechanism in diagnostic.cc and I don't
think this is something that we can add a test for there. Let me know if
I'm wrong.
Successfully tested on x86_64-pc-linux-gnu. OK for GCC 16?
PR c++/118163
PR c++/118392
gcc/ChangeLog:
* diagnostic.cc (diagnostic_context::initialize): Initialize
m_diagnostic_groups.m_inhibiting_notes_from.
(diagnostic_context::inhibit_notes_in_group): New.
(diagnostic_context::notes_inhibited_in_group): New
(diagnostic_context::report_diagnostic): Call
inhibit_notes_in_group and notes_inhibited_in_group.
(diagnostic_context::end_group): Call inhibit_notes_in_group.
(diagnostic_context::pop_nesting_level): Ditto.
* diagnostic.h (diagnostic_context::m_diagnostic_groups): Add
member to track the depth at which a warning has been inhibited.
(diagnostic_context::notes_inhibited_in_group): Declare.
(diagnostic_context::inhibit_notes_in_group): Declare.
* doc/ux.texi: Document diagnostic_group behavior with regards
to inhibited warnings.
gcc/testsuite/ChangeLog:
* g++.dg/diagnostic/incomplete-type-2.C: New test.
* g++.dg/diagnostic/incomplete-type-2a.C: New test.
* g++.dg/diagnostic/inhibit-warn-3.C: New test.
---
gcc/diagnostic.cc | 67 ++++++++++++++++++-
gcc/diagnostic.h | 6 ++
gcc/doc/ux.texi | 5 +-
.../g++.dg/diagnostic/incomplete-type-2.C | 7 ++
.../g++.dg/diagnostic/incomplete-type-2a.C | 13 ++++
.../g++.dg/diagnostic/inhibit-warn-3.C | 15 +++++
6 files changed, 110 insertions(+), 3 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/diagnostic/incomplete-type-2.C
create mode 100644 gcc/testsuite/g++.dg/diagnostic/incomplete-type-2a.C
create mode 100644 gcc/testsuite/g++.dg/diagnostic/inhibit-warn-3.C
diff --git a/gcc/diagnostic.cc b/gcc/diagnostic.cc
index c2f6714c24a..310497f4309 100644
--- a/gcc/diagnostic.cc
+++ b/gcc/diagnostic.cc
@@ -283,6 +283,7 @@ diagnostic_context::initialize (int n_opts)
m_diagnostic_groups.m_group_nesting_depth = 0;
m_diagnostic_groups.m_diagnostic_nesting_level = 0;
m_diagnostic_groups.m_emission_count = 0;
+ m_diagnostic_groups.m_inhibiting_notes_from = 0;
m_output_sinks.safe_push
(new diagnostic_text_output_format (*this, nullptr, true));
m_set_locations_cb = nullptr;
@@ -891,6 +892,7 @@ diagnostic_context::check_max_errors (bool flush)
/* Take any action which is expected to happen after the diagnostic
is written out. This function does not always return. */
+
void
diagnostic_context::action_after_output (diagnostic_t diag_kind)
{
@@ -965,6 +967,50 @@ diagnostic_context::action_after_output (diagnostic_t
diag_kind)
}
}
+/* State whether we should inhibit notes in the current diagnostic_group and
+ its future children if any. */
+
+void
+diagnostic_context::inhibit_notes_in_group (bool inhibit)
+{
+ int curr_depth = (m_diagnostic_groups.m_group_nesting_depth
+ + m_diagnostic_groups.m_diagnostic_nesting_level);
+
+ if (inhibit)
+ {
+ /* If we're already inhibiting, there's nothing to do. */
+ if (m_diagnostic_groups.m_inhibiting_notes_from)
+ return;
+
+ /* Since we're called via warning/error/... that all have their own
+ diagnostic_group, we must consider that we started inhibiting in their
+ parent. */
+ gcc_assert (m_diagnostic_groups.m_group_nesting_depth > 0);
+ m_diagnostic_groups.m_inhibiting_notes_from = curr_depth - 1;
+ }
+ else if (m_diagnostic_groups.m_inhibiting_notes_from)
+ {
+ /* Only cancel inhibition at the depth that set it up. */
+ if (curr_depth >= m_diagnostic_groups.m_inhibiting_notes_from)
+ return;
+
+ m_diagnostic_groups.m_inhibiting_notes_from = 0;
+ }
+}
+
+/* Return whether notes must be inhibited in the current diagnostic_group. */
+
+bool
+diagnostic_context::notes_inhibited_in_group () const
+{
+ if (m_diagnostic_groups.m_inhibiting_notes_from
+ && (m_diagnostic_groups.m_group_nesting_depth
+ + m_diagnostic_groups.m_diagnostic_nesting_level
+ >= m_diagnostic_groups.m_inhibiting_notes_from))
+ return true;
+ return false;
+}
+
/* class logical_location. */
/* Return true iff this is a function or method. */
@@ -1355,7 +1401,10 @@ diagnostic_context::report_diagnostic (diagnostic_info
*diagnostic)
bool was_warning = (diagnostic->kind == DK_WARNING
|| diagnostic->kind == DK_PEDWARN);
if (was_warning && m_inhibit_warnings)
- return false;
+ {
+ inhibit_notes_in_group ();
+ return false;
+ }
if (m_adjust_diagnostic_info)
m_adjust_diagnostic_info (this, diagnostic);
@@ -1397,7 +1446,10 @@ diagnostic_context::report_diagnostic (diagnostic_info
*diagnostic)
not disabled by #pragma GCC diagnostic anywhere along the inlining
stack. . */
if (!diagnostic_enabled (diagnostic))
- return false;
+ {
+ inhibit_notes_in_group ();
+ return false;
+ }
if ((was_warning || diagnostic->kind == DK_WARNING)
&& ((!m_warn_system_headers
@@ -1407,9 +1459,16 @@ diagnostic_context::report_diagnostic (diagnostic_info
*diagnostic)
inlining stack (if there is one) are in system headers. */
return false;
+ if (diagnostic->kind == DK_NOTE && notes_inhibited_in_group ())
+ /* Bail for all the notes in the diagnostic_group that started to inhibit
notes. */
+ return false;
+
if (diagnostic->kind != DK_NOTE && diagnostic->kind != DK_ICE)
check_max_errors (false);
+ /* We are accepting the diagnostic, so should stop inhibiting notes. */
+ inhibit_notes_in_group (/*inhibit=*/false);
+
m_lock++;
if (diagnostic->kind == DK_ICE || diagnostic->kind == DK_ICE_NOBT)
@@ -1743,6 +1802,8 @@ diagnostic_context::end_group ()
sink->on_end_group ();
m_diagnostic_groups.m_emission_count = 0;
}
+ /* We're popping one level, so might need to stop inhibiting notes. */
+ inhibit_notes_in_group (/*inhibit=*/false);
}
void
@@ -1755,6 +1816,8 @@ void
diagnostic_context::pop_nesting_level ()
{
--m_diagnostic_groups.m_diagnostic_nesting_level;
+ /* We're popping one level, so might need to stop inhibiting notes. */
+ inhibit_notes_in_group (/*inhibit=*/false);
}
void
diff --git a/gcc/diagnostic.h b/gcc/diagnostic.h
index 202760b2f85..74ec61fe5ec 100644
--- a/gcc/diagnostic.h
+++ b/gcc/diagnostic.h
@@ -950,8 +950,14 @@ private:
/* How many diagnostics have been emitted since the bottommost
diagnostic_group was pushed. */
int m_emission_count;
+
+ /* The "group+diagnostic" nesting depth from which to inhibit notes. */
+ int m_inhibiting_notes_from;
} m_diagnostic_groups;
+ void inhibit_notes_in_group (bool inhibit = true);
+ bool notes_inhibited_in_group () const;
+
/* The various sinks to which diagnostics are to be outputted
(text vs structured formats such as SARIF).
The sinks are owned by the context; this would be a
diff --git a/gcc/doc/ux.texi b/gcc/doc/ux.texi
index 2428d1a68f9..243b3e08b11 100644
--- a/gcc/doc/ux.texi
+++ b/gcc/doc/ux.texi
@@ -409,7 +409,10 @@ diagnostic subsystem that all diagnostics issued within
the lifetime
of the @code{auto_diagnostic_group} are related. For example,
@option{-fdiagnostics-format=json} will treat the first diagnostic
emitted within the group as a top-level diagnostic, and all subsequent
-diagnostics within the group as its children.
+diagnostics within the group as its children. Also, if a warning in the
+group is inhibited at nesting depth D, all subsequent notes at that depth
+or deeper will be inhibited as well, until an error or another warning
+is emitted, the depth decreases below D, or the group is popped.
@subsection Quoting
Text should be quoted by either using the @samp{q} modifier in a directive
diff --git a/gcc/testsuite/g++.dg/diagnostic/incomplete-type-2.C
b/gcc/testsuite/g++.dg/diagnostic/incomplete-type-2.C
new file mode 100644
index 00000000000..e2fb20a4ae8
--- /dev/null
+++ b/gcc/testsuite/g++.dg/diagnostic/incomplete-type-2.C
@@ -0,0 +1,7 @@
+// PR c++/118163
+// { dg-do "compile" }
+
+template<class T>
+struct S { // { dg-note "until the closing brace" }
+ S s; // { dg-error "has incomplete type" }
+};
diff --git a/gcc/testsuite/g++.dg/diagnostic/incomplete-type-2a.C
b/gcc/testsuite/g++.dg/diagnostic/incomplete-type-2a.C
new file mode 100644
index 00000000000..d13021d0b68
--- /dev/null
+++ b/gcc/testsuite/g++.dg/diagnostic/incomplete-type-2a.C
@@ -0,0 +1,13 @@
+// PR c++/118163
+// { dg-do "compile" }
+// { dg-additional-options "-Wno-template-body" }
+
+template<class T>
+struct S { // { dg-bogus "until the closing brace" }
+ S s; // { dg-bogus "has incomplete type" }
+};
+
+// Check that we don't suppress errors outside of the body.
+struct forward_decl; // { dg-note "forward declaration" }
+template<class T>
+void foo (forward_decl) {} // { dg-error "has incomplete type" }
diff --git a/gcc/testsuite/g++.dg/diagnostic/inhibit-warn-3.C
b/gcc/testsuite/g++.dg/diagnostic/inhibit-warn-3.C
new file mode 100644
index 00000000000..bd021df5da4
--- /dev/null
+++ b/gcc/testsuite/g++.dg/diagnostic/inhibit-warn-3.C
@@ -0,0 +1,15 @@
+// PR c++/118392
+// { dg-do "compile" }
+// { dg-additional-options "-w" }
+
+// Fake dg-note to make sure that notes are not pruned and we can use dg-bogus.
+// { dg-note "fake" "" { xfail *-*-* } }
+
+namespace xxx {
+ struct foo {
+ friend void bar(); // { dg-bogus "only here as a friend" }
+ };
+}
+void xxx::bar () {}
+
+void foo () {}
--
2.44.0
