From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 45933 invoked by alias); 3 Jul 2017 16:25:11 -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 45906 invoked by uid 89); 3 Jul 2017 16:25:10 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-25.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_LAZY_DOMAIN_SECURITY,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=imply, front-end X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 03 Jul 2017 16:25:08 +0000 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 6B2B24ACCC for ; Mon, 3 Jul 2017 16:25:07 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 6B2B24ACCC Authentication-Results: ext-mx09.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx09.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=law@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 6B2B24ACCC Received: from localhost.localdomain (ovpn-117-103.phx2.redhat.com [10.3.117.103]) by smtp.corp.redhat.com (Postfix) with ESMTP id 363025D960; Mon, 3 Jul 2017 16:25:07 +0000 (UTC) Subject: Re: [PATCH 1/3] c-family: add name_hint/deferred_diagnostic To: David Malcolm , gcc-patches@gcc.gnu.org References: <1494006675-28033-1-git-send-email-dmalcolm@redhat.com> From: Jeff Law Message-ID: <7018d80f-e19a-4f9f-d8d0-380d49cdbabb@redhat.com> Date: Mon, 03 Jul 2017 16:25:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.1.0 MIME-Version: 1.0 In-Reply-To: <1494006675-28033-1-git-send-email-dmalcolm@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-IsSubscribed: yes X-SW-Source: 2017-07/txt/msg00123.txt.bz2 On 05/05/2017 11:51 AM, David Malcolm wrote: > In various places we use lookup_name_fuzzy to provide a hint, > and can report messages of the form: > error: unknown foo named 'bar' > or: > error: unknown foo named 'bar'; did you mean 'SUGGESTION? > > This patch provides a way for lookup_name_fuzzy to provide > both the suggestion above, and (optionally) additional hints > that can be printed e.g. > > note: did you forget to include ? > > This patch provides the mechanism and ports existing users > of lookup_name_fuzzy to the new return type. > There are no uses of such hints in this patch, but followup > patches provide various front-end specific uses of this. > > gcc/c-family/ChangeLog: > * c-common.h (class deferred_diagnostic): New class. > (class name_hint): New class. > (lookup_name_fuzzy): Convert return type from const char * > to name_hint. Add location_t param. > > gcc/c/ChangeLog: > * c-decl.c (implicit_decl_warning): Convert "hint" from > const char * to name_hint. Pass location to > lookup_name_fuzzy. Suppress any deferred diagnostic if the > warning was not printed. > (undeclared_variable): Likewise for "guessed_id". > (lookup_name_fuzzy): Convert return type from const char * > to name_hint. Add location_t param. > * c-parser.c (c_parser_declaration_or_fndef): Convert "hint" from > const char * to name_hint. Pass location to lookup_name_fuzzy. > (c_parser_parameter_declaration): Pass location to > lookup_name_fuzzy. > > gcc/cp/ChangeLog: > * name-lookup.c (suggest_alternatives_for): Convert "fuzzy_name" from > const char * to name_hint, and rename to "hint". Pass location to > lookup_name_fuzzy. > (lookup_name_fuzzy): Convert return type from const char * > to name_hint. Add location_t param. > * parser.c (cp_parser_diagnose_invalid_type_name): Convert > "suggestion" from const char * to name_hint, and rename to "hint". > Pass location to lookup_name_fuzzy. > --- > gcc/c-family/c-common.h | 121 +++++++++++++++++++++++++++++++++++++++++++++++- > gcc/c/c-decl.c | 35 +++++++------- > gcc/c/c-parser.c | 16 ++++--- > gcc/cp/name-lookup.c | 17 +++---- > gcc/cp/parser.c | 12 ++--- > 5 files changed, 163 insertions(+), 38 deletions(-) > > diff --git a/gcc/c-family/c-common.h b/gcc/c-family/c-common.h > index 138a0a6..83c1a68 100644 > --- a/gcc/c-family/c-common.h > +++ b/gcc/c-family/c-common.h > @@ -1009,7 +1009,126 @@ enum lookup_name_fuzzy_kind { > /* Any name. */ > FUZZY_LOOKUP_NAME > }; > -extern const char *lookup_name_fuzzy (tree, enum lookup_name_fuzzy_kind); > + > +/* A deferred_diagnostic is a wrapper around optional extra diagnostics > + that we may want to bundle into a name_hint. > + > + The emit method is called when no name_hint instances reference > + the deferred_diagnostic. In the simple case this is when the name_hint > + goes out of scope, but a reference-counting scheme is used to allow > + name_hint instances to be copied. */ > + > +class deferred_diagnostic > +{ > + public: > + virtual ~deferred_diagnostic () {} > + virtual void emit () = 0; > + > + void incref () { m_refcnt++; } > + void decref () > + { > + if (--m_refcnt == 0) > + { > + if (!m_suppress) > + emit (); > + delete this; > + } > + } > + > + location_t get_location () const { return m_loc; } > + > + /* Call this if the corresponding warning was not emitted, > + in which case we should also not emit the deferred_diagnostic. */ > + void suppress () > + { > + m_suppress = true; > + } > + > + protected: > + deferred_diagnostic (location_t loc) > + : m_refcnt (0), m_loc (loc), m_suppress (false) {} > + > + private: > + int m_refcnt; > + location_t m_loc; > + bool m_suppress; > +}; So what stands out here is "delete this" and the need for explicit reference counting. Also doesn't that imply that deferred_diagnostic objects must be allocated on the heap? Is there another way to get the behavior you want without resorting to something like this? Or is your argument that deferred_diagnostic is only used from within class name_hint and thus the concerns around heap vs stack, explicit counting, etc are all buried inside the name_hint class? If so, is there any reasonable way to restrict the use of deferred_disagnostic to within the name_hint class? The rest of the changes seem non-controversial, so I think if we can sort out the issues with those classes then this will be fine to move forward. jeff