On Wed, 2018-12-05 at 19:49 -0500, Jason Merrill wrote:
On 12/3/18 5:54 PM, David Malcolm wrote:
[...]
Thanks for the review. Here's a v3 of the patch; comments inline
below.
diff --git a/gcc/cp/call.c b/gcc/cp/call.c
index ee099cc..cfc5641 100644
--- a/gcc/cp/call.c
+++ b/gcc/cp/call.c
@@ -6681,16 +6681,24 @@ conversion_null_warnings (tree totype, tree
expr, tree fn, int argnum)
if (null_node_p (expr) && TREE_CODE (totype) != BOOLEAN_TYPE
&& ARITHMETIC_TYPE_P (totype))
{
- location_t loc =
- expansion_point_location_if_in_system_header
(input_location);
-
if (fn)
- warning_at (loc, OPT_Wconversion_null,
- "passing NULL to non-pointer argument %P of
%qD",
- argnum, fn);
+ {
+ location_t loc = EXPR_LOC_OR_LOC (expr, input_location);
+ loc = expansion_point_location_if_in_system_header
(loc);
+ auto_diagnostic_group d;
+ if (warning_at (loc, OPT_Wconversion_null,
+ "passing NULL to non-pointer argument %P
of %qD",
+ argnum, fn))
+ inform (get_fndecl_argument_location (fn, argnum),
+ " declared here");
+ }
else
- warning_at (loc, OPT_Wconversion_null,
- "converting to non-pointer type %qT from
NULL", totype);
+ {
+ location_t loc
+ = expansion_point_location_if_in_system_header
(input_location);
+ warning_at (loc, OPT_Wconversion_null,
+ "converting to non-pointer type %qT from
NULL", totype);
+ }
Why is 'loc' different between the branches?
Good catch; I've consolidated them in the v3 patch.
@@ -6698,9 +6706,15 @@ conversion_null_warnings (tree totype, tree
expr, tree fn, int argnum)
&& TYPE_PTR_P (totype))
{
if (fn)
- warning_at (input_location, OPT_Wconversion_null,
- "converting %<false%> to pointer type for
argument %P "
- "of %qD", argnum, fn);
+ {
+ location_t loc = EXPR_LOC_OR_LOC (expr, input_location);
+ auto_diagnostic_group d;
+ if (warning_at (loc, OPT_Wconversion_null,
+ "converting %<false%> to pointer type
for argument "
+ "%P of %qD", argnum, fn))
+ inform (get_fndecl_argument_location (fn, argnum),
+ " declared here");
+ }
else
warning_at (input_location, OPT_Wconversion_null,
"converting %<false%> to pointer type %qT",
totype);
Same question.
Likewise.
@@ -6740,6 +6754,15 @@ maybe_print_user_conv_context (conversion
*convs)
location_t
get_fndecl_argument_location (tree fndecl, int argnum)
{
+ /* Gracefully fail for e.g. TEMPLATE_DECL. */
+ if (TREE_CODE (fndecl) != FUNCTION_DECL)
+ return DECL_SOURCE_LOCATION (fndecl);
For a TEMPLATE_DECL we can use DECL_TEMPLATE_RESULT or STRIP_TEMPLATE
to
get the FUNCTION_DECL. But I'm somewhat surprised we would get here
with a TEMPLATE_DECL rather than an instantiation.
FWIW I hit this for e.g. g++.dg/diagnostic/missing-default-args.C within
the new code in check_default_args, when reporting on an template
with a param with no-default-args following a param with default args.
In the v3 patch I've removed the check for FUNCTION_DECL, in favor of
a STRIP_TEMPLATE within check_default_args.
diff --git a/gcc/cp/decl2.c b/gcc/cp/decl2.c
index ffc0d0d..265826a 100644
--- a/gcc/cp/decl2.c
+++ b/gcc/cp/decl2.c
@@ -48,6 +48,7 @@ along with GCC; see the file COPYING3. If not
see
#include "intl.h"
#include "c-family/c-ada-spec.h"
#include "asan.h"
+#include "gcc-rich-location.h"
/* Id for dumping the raw trees. */
int raw_dump_id;
@@ -5179,14 +5180,25 @@ check_default_args (tree x)
{
tree arg = TYPE_ARG_TYPES (TREE_TYPE (x));
bool saw_def = false;
+ location_t loc_of_first_default_arg = UNKNOWN_LOCATION;
int i = 0 - (TREE_CODE (TREE_TYPE (x)) == METHOD_TYPE);
for (; arg && arg != void_list_node; arg = TREE_CHAIN (arg),
++i)
{
if (TREE_PURPOSE (arg))
- saw_def = true;
+ {
+ saw_def = true;
+ location_t loc = get_fndecl_argument_location (x, i);
+ if (loc != DECL_SOURCE_LOCATION (x))
+ loc_of_first_default_arg = loc;
+ }
else if (saw_def && !PACK_EXPANSION_P (TREE_VALUE (arg)))
{
- error ("default argument missing for parameter %P of
%q+#D", i, x);
+ location_t loc = get_fndecl_argument_location (x, i);
+ gcc_rich_location richloc (loc);
+ if (loc_of_first_default_arg != UNKNOWN_LOCATION)
+ richloc.add_range (loc_of_first_default_arg);
+ error_at (&richloc,
+ "default argument missing for parameter %P of
%q#D", i, x);
If we're going to highlight the earlier parameter that has a default
argument, we should mention in the diagnostic why it's relevant.
Yeah, the secondary location wasn't very self-explanatory.
In the v3 patch I've changed it to emit a note (once per fndecl)
highlighting the first param that has a default argument.
Given e.g.:
void test (int a, int b = 42, int c=1776, int d, int e);
With trunk we have:
demo.cc:5:6: error: default argument missing for parameter 4 of
'void test(int, int, int, int, int)'
5 | void test (int a, int b = 42, int c=1776, int d, int e);
| ^~~~
demo.cc:5:6: error: default argument missing for parameter 5 of
'void test(int, int, int, int, int)'
With the v3 patch it emits:
demo.cc:5:47: error: default argument missing for parameter 4 of
'void test(int, int, int, int, int)'
5 | void test (int a, int b = 42, int c=1776, int d, int e);
| ~~~~^
demo.cc:5:23: note: ...following parameter 2 which has a default
argument
5 | void test (int a, int b = 42, int c=1776, int d, int e);
| ~~~~^~~~~~
demo.cc:5:54: error: default argument missing for parameter 5 of
'void test(int, int, int, int, int)'
5 | void test (int a, int b = 42, int c=1776, int d, int e);
| ~~~~^
thus highlighting which specific params we mean, and giving a
hint to the user about why we're complaining. Maybe the
wording could use some improvement (the above is about as terse
as I felt I could make it without it becoming hard to read).
Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu.
OK for trunk?
