From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 78735 invoked by alias); 10 Jul 2017 12:09:28 -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 77108 invoked by uid 89); 10 Jul 2017 12:09:27 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No 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,RP_MATCHES_RCVD,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.2 spammy=concrete, buried X-HELO: paperclip.tbsaunde.org Received: from tbsaunde.org (HELO paperclip.tbsaunde.org) (66.228.47.254) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 10 Jul 2017 12:09:25 +0000 Received: from ball (pool-98-116-146-128.nycmny.fios.verizon.net [98.116.146.128]) by paperclip.tbsaunde.org (Postfix) with ESMTPSA id 3915AC06B; Mon, 10 Jul 2017 12:09:23 +0000 (UTC) Date: Mon, 10 Jul 2017 12:09:00 -0000 From: Trevor Saunders To: David Malcolm Cc: Jeff Law , gcc-patches@gcc.gnu.org Subject: Re: [PATCH 1/3] c-family: add name_hint/deferred_diagnostic Message-ID: <20170710120922.wwawk6k7v2yd7yvl@ball> References: <1494006675-28033-1-git-send-email-dmalcolm@redhat.com> <7018d80f-e19a-4f9f-d8d0-380d49cdbabb@redhat.com> <1499103166.7656.83.camel@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1499103166.7656.83.camel@redhat.com> User-Agent: NeoMutt/20170609 (1.8.3) X-SW-Source: 2017-07/txt/msg00456.txt.bz2 On Mon, Jul 03, 2017 at 01:32:46PM -0400, David Malcolm wrote: > On Mon, 2017-07-03 at 10:25 -0600, Jeff Law wrote: > > 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? > > > > Thanks for looking at this. > > Yes: deferred_diagnostic instances are heap-allocated. This is because > it's an abstract base class; each concrete subclass is an > implementation detail within the frontends, for isolating the special > -case logic for each different kind of hint, and thus these concrete > subclasses are hidden within the FE code. > > My initial implementation of the above had the name_hint class directly > "own" the deferred_diagnostic ptr, with a: > delete m_deferred; > within name_hint's dtor. > > This worked OK, until I encountered places in the C and C++ frontends > where the natural thing (to avoid complicating the control flow) was to > have a default-constructed name_hint as we enter the error-handling, > and then optionally copy over it with a name_hint instance returned > from a call to lookup_name_fuzzy on some of the paths (specifically, > this was in c/c-decl.c's implicit_decl_warning and in and > cp/parser.c's cp_parser_diagnose_invalid_type_name). > > AIUI, the idiomatic way to do this assignment of an "owned" ptr in > modern C++ would be to use std::unique_ptr (if I understand things > right, this is move-asssignment), the issue being that when we return a > name_hint: That would be one way, though you can avoid allocating on the heap by doing something like this. std::option hint; if (something) foo (&something); void foo(std::option *hint) { hint->emplace (name_hint (5)); } > name_hint hint; > if (sometimes) > hint = lookup_name_fuzzy (...); > // potentially use hint > // hint's dtor runs, showing any deferred diagnostic it "owns" > > we have two name_hint instances: the return value, and the local > "hint", and the return value's dtor runs immediately after the local is > assigned to, but before we use the deferred diagnostic. > > I believe that the above with a std::unique_ptr within class name_hint > requires the "move semantics" of C+11 in order to be guaranteed to work > correctly, which is beyond what we currently require (C++98 iirc). Well, you can use something like std::auto_ptr that makes this "work" without move assignment by having a copy constructor that modifies the original object. I have patches to add something like that that I need to get to posting, but have been delayed by moving and such things. > Hence I coded up something akin to std::shared_ptr "by hand" here (and > yes, it is perhaps overkill). fwiw when its necessary to refcount things I do prefer "intrusive" refcounting like this to the way shared_ptr does it. > > 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? > > I guess I could try moving the scope of the name deferred_diagnostic to > be within class name_hint. > > Other approaches to simplify the code: > > (a) GC-allocate the deferred_diagnostic, and let it be collected next > time the GC collects (as nothing would ever have a root to it). I'd prefer the current approach to this the refcounting code is ugly, though perhaps we'll need a generic refcounting framework some day. However I think its easier to reason about ownership with refcounting than gc, and hopefully make it easier to switch to something like unique_ptr when I finish those patches. > (b) convert the deferred_diagnostic subclasses to be singletons, and > then return a ptr to the singleton (and clobber them each time we need > a supposedly "new" one). This also seems to really complicate ownership which is more of a global problem than the refcounting? > Alternatively, we could hide the copy-assignment operator for > name_hint, and uglify/complicate the control flow in the two sites that > need it. > > Any other ideas? I think my highest preference would be the std::option thing, though I'm not sure we have code for that yet. Then would be the unique_ptr using auto_ptr approach I'm working on, and then what you have here. With the assumption we'll switch it to one of the first two when possible. thanks Trev