This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] v3: C++: improvements to diagnostics using %P (more PR c++/85110)


On 12/6/18 10:38 AM, David Malcolm wrote:
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?

OK.

Jason


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]