On 6/23/21 11:26 PM, Jeff Law wrote:
On 6/23/2021 1:47 PM, Martin Sebor via Gcc-patches wrote:
On 6/22/21 5:28 PM, David Malcolm wrote:
On Tue, 2021-06-22 at 19:18 -0400, David Malcolm wrote:
On Fri, 2021-06-04 at 15:41 -0600, Martin Sebor wrote:
The attached patch introduces the suppress_warning(),
warning_suppressed(), and copy_no_warning() APIs without making
use of them in the rest of GCC. They are in three files:
diagnostic-spec.{h,c}: Location-centric overloads.
warning-control.cc: Tree- and gimple*-centric overloads.
The location-centric overloads are suitable to use from the
diagnostic
subsystem. The rest can be used from the front ends and the middle
end.
[...snip...]
+/* Return true if warning OPT is suppressed for decl/expression
EXPR.
+ By default tests the disposition for any warning. */
+
+bool
+warning_suppressed_p (const_tree expr, opt_code opt /* =
all_warnings */)
+{
+ const nowarn_spec_t *spec = get_nowarn_spec (expr);
+
+ if (!spec)
+ return get_no_warning_bit (expr);
+
+ const nowarn_spec_t optspec (opt);
+ bool dis = *spec & optspec;
+ gcc_assert (get_no_warning_bit (expr) || !dis);
+ return dis;
Looking through the patch, I don't like the use of "dis" for the "is
suppressed" bool, since...
It's an argument to a a function, unlikely to confuse anyone. But
I also don't think it's important enough to argue about so I changed
it along with the comment below.
[...snip...]
+
+/* Enable, or by default disable, a warning for the statement STMT.
+ The wildcard OPT of -1 controls all warnings. */
...I find the above comment to be confusingly worded due to the double-
negative.
If I'm reading your intent correctly, how about this wording:
/* Change the supression of warnings for statement STMT.
OPT controls which warnings are affected.
The wildcard OPT of -1 controls all warnings.
If SUPP is true (the default), enable the suppression of the
warnings.
If SUPP is false, disable the suppression of the warnings. */
...and rename "dis" to "supp".
Or have I misread the intent of the patch?
+
+void
+suppress_warning (gimple *stmt, opt_code opt /* = all_warnings */,
+ bool dis /* = true */)
+{
+ if (opt == no_warning)
+ return;
+
+ const key_type_t key = convert_to_key (stmt);
+
+ dis = suppress_warning_at (key, opt, dis) || dis;
+ set_no_warning_bit (stmt, dis);
+}
[...snip...]
Also, I think I prefer having a separate entrypoints for the "all
warnings" case; on reading through the various patches I think that in
e.g.:
- TREE_NO_WARNING (*expr_p) = 1;
+ suppress_warning (*expr_p);
I prefer:
suppress_warnings (*expr_p);
(note the plural) since that way we can grep for them, and it seems
like better grammar to me.
Both entrypoints could be implemented by a static suppress_warning_1
internally if that makes it easier.
In that vein, "unsuppress_warning" seems far clearer to me that
"suppress_warning (FOO, false)"; IIRC there are very few uses of this
non-default arg (I couldn't find any in a quick look through the v2
kit).
Does this make sense?
In my experience, names that differ only slightly (such as in just
one letter) tend to easily lead to frustrating mistakes. The new
warning_suppressed_p() added in this patch also handles multiple
warnings (via a wildcard) and uses a singular form, and as do
the gimple_{get,set}no_warning() functions that are being replaced.
So I don't think the suggested change is needed or would be
an improvement.
Similarly, there is no gimple_unset_no_warning() today and I don't
think adding unsuppress_warning() is necessary or would add value,
especially since there are just a handful of callers that re-enable
warnings. For the callers that might need to enable or disable
a warning based on some condition, it's more convenient to do so
in a single call then by having to call different functions based
the value of the condition.
In the attached patch I've changed the comment as you asked and
also renamed the dis argument to supp but kept the function names
the same. I've also moved a few changes to tree.h from a later
patch to this one to let it stand alone and regtested it on its
own on x86_64-linux. I'll plan to go ahead with this version
unless requestsfor substantive changes come up in the review of
the rest of the changes.
Thanks
Martin
gcc-no-warning-1.diff
Add support for per-location warning groups.
gcc/ChangeLog:
* Makefile.in (OBJS-libcommon): Add diagnostic-spec.o.
* gengtype.c (open_base_files): Add diagnostic-spec.h.
* diagnostic-spec.c: New file.
* diagnostic-spec.h: New file.
* tree.h (no_warning, all_warnings, suppress_warning_at): New
declarations.
* warning-control.cc: New file.
diff --git a/gcc/diagnostic-spec.h b/gcc/diagnostic-spec.h
new file mode 100644
index 00000000000..62d9270ca6d
--- /dev/null
+++ b/gcc/diagnostic-spec.h
@@ -0,0 +1,140 @@
+/* Language-independent APIs to enable/disable per-location warnings.
+
+ Copyright (C) 2021 Free Software Foundation, Inc.
+ Contributed by Martin Sebor<mse...@redhat.com>
+
+ This file is part of GCC.
+
+ GCC is free software; you can redistribute it and/or modify it under
+ the terms of the GNU General Public License as published by the Free
+ Software Foundation; either version 3, or (at your option) any later
+ version.
+
+ GCC is distributed in the hope that it will be useful, but WITHOUT ANY
+ WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
+ for more details.
+
+ You should have received a copy of the GNU General Public License
+ along with GCC; see the file COPYING3. If not see
+<http://www.gnu.org/licenses/>. */
+
+#ifndef DIAGNOSTIC_SPEC_H_INCLUDED
+#define DIAGNOSTIC_SPEC_H_INCLUDED
+
+#include "hash-map.h"
+
+/* A "bitset" of warning groups. */
+
+struct nowarn_spec_t
Should probably be "class" not "struct". Though I don't think we're
necessarily consistent with this.
+private:
+ /* Bitset of warning groups. */
+ unsigned bits;
Our conventions are to prefix member fields with m_.
Note that a simple "unsigned" may just be 32 bits. Though with your
grouping that may not matter in practice.
I think with the nits fixed this is fine. I know there's some
disagreement on singular vs plural, but having two names differing by
just that is probably going to be more confusing in the end, so I'm on
the side of using the names as-is.
Done and pushed in r12-1801.
Martin
