From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 108442 invoked by alias); 6 Dec 2018 15:04:05 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 108420 invoked by uid 89); 6 Dec 2018 15:04:04 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.2 spammy= X-HELO: mail-qk1-f193.google.com Received: from mail-qk1-f193.google.com (HELO mail-qk1-f193.google.com) (209.85.222.193) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 06 Dec 2018 15:04:00 +0000 Received: by mail-qk1-f193.google.com with SMTP id r71so449979qkr.10 for ; Thu, 06 Dec 2018 07:04:00 -0800 (PST) Return-Path: Received: from [192.168.1.149] (209-6-216-142.s141.c3-0.smr-cbr1.sbo-smr.ma.cable.rcncustomer.com. [209.6.216.142]) by smtp.gmail.com with ESMTPSA id a90sm442428qka.30.2018.12.06.07.03.57 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 06 Dec 2018 07:03:57 -0800 (PST) Subject: Re: [PATCH] v3: C++: improvements to diagnostics using %P (more PR c++/85110) To: David Malcolm Cc: gcc-patches@gcc.gnu.org References: <9b627d17-0203-3b5a-7257-790b8243156c@redhat.com> <1544110738-40640-1-git-send-email-dmalcolm@redhat.com> From: Jason Merrill Message-ID: Date: Thu, 06 Dec 2018 15:04:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.3.1 MIME-Version: 1.0 In-Reply-To: <1544110738-40640-1-git-send-email-dmalcolm@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2018-12/txt/msg00370.txt.bz2 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 % 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 % 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 % 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