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

[Jason Merrill <jason@redhat.com>]

On 12/3/18 5:54 PM, David Malcolm wrote:
> I was going to ping this patch:
>    https://gcc.gnu.org/ml/gcc-patches/2018-11/msg00875.html
> but it has bit-rotted somewhat, so here's a refreshed version of it.
> 
> Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu.
> 
> OK for trunk?
> 
> Thanks
> Dave
> 
> 
> Blurb from v1:
> 
> This patch is based on grepping the C++ frontend for %P
> i.e. diagnostics that refer to a parameter number.  It fixes up
> these diagnostics to highlight the pertinent param where appropriate
> (and possible), along with various other tweaks, as described in the
> ChangeLog.
> 
> gcc/cp/ChangeLog:
> 	PR c++/85110
> 	* call.c (conversion_null_warnings): Try to use the location of
> 	the expression for the warnings.  Add notes showing the parameter
> 	of the function decl, where available.
> 	(get_fndecl_argument_location): Gracefully reject
> 	non-FUNCTION_DECLs.  For implicitly-declared functions, use the
> 	fndecl location rather than that of the param.
> 	(maybe_inform_about_fndecl_for_bogus_argument_init): New function.
> 	(convert_like_real): Use it in various places to avoid repetition.
> 	* cp-tree.h (maybe_inform_about_fndecl_for_bogus_argument_init):
> 	New declaration.
> 	* decl2.c: Include "gcc-rich-location.h".
> 	(check_default_args): Use the location of the parameter when
> 	complaining about parameters with missing default arguments in
> 	preference to that of the fndecl.
> 	Attempt to record the location of first parameter with a default
> 	argument and add it as a secondary range to such errors.
> 	* typeck.c (convert_arguments): When complaining about parameters
> 	with incomplete types, attempt to use the location of the
> 	argument.  Where available, add a note showing the pertinent
> 	parameter in the fndecl.
> 	(convert_for_assignment): When complaining about bad conversions
> 	at function calls, use the location of the unstripped argument.
> 	(convert_for_initialization): When checking for bogus references,
> 	add an auto_diagnostic_group, and update the note to use the
> 	location of the pertinent parameter, rather than just the callee.
> 
> gcc/testsuite/ChangeLog:
> 	PR c++/85110
> 	* g++.dg/diagnostic/missing-default-args.C: New test.
> 	* g++.dg/diagnostic/param-type-mismatch-3.C: New test.
> 	* g++.dg/diagnostic/param-type-mismatch.C: Add tests for invalid
> 	references and incomplete types.
> 	* g++.dg/warn/Wconversion-null-4.C: New test.
> ---
>   gcc/cp/call.c                                      | 88 ++++++++++++++--------
>   gcc/cp/cp-tree.h                                   |  1 +
>   gcc/cp/decl2.c                                     | 16 +++-
>   gcc/cp/typeck.c                                    | 22 ++++--
>   .../g++.dg/diagnostic/missing-default-args.C       | 53 +++++++++++++
>   .../g++.dg/diagnostic/param-type-mismatch-3.C      | 26 +++++++
>   .../g++.dg/diagnostic/param-type-mismatch.C        | 41 ++++++++++
>   gcc/testsuite/g++.dg/warn/Wconversion-null-4.C     | 43 +++++++++++
>   8 files changed, 251 insertions(+), 39 deletions(-)
>   create mode 100644 gcc/testsuite/g++.dg/diagnostic/missing-default-args.C
>   create mode 100644 gcc/testsuite/g++.dg/diagnostic/param-type-mismatch-3.C
>   create mode 100644 gcc/testsuite/g++.dg/warn/Wconversion-null-4.C
> 
> 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?

> @@ -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.

> @@ -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.

> 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.

Jason