On 07/03/2017 11:32 AM, 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 <SOME_HEADER.h>? >>> >>> 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: > > 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. Right. And raises the general issue of what to do with "owned" external resources such as allocated memory.
> > 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). > > [C++ gurus, please correct me if I've got this wrong] > > Hence I coded up something akin to std::shared_ptr "by hand" here (and > yes, it is perhaps overkill). > >> 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). > > (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). > > 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 don't have strong opinions in this space -- primarily I noticed the reference counting and explicit deletion which seemed unusual and would likely have implications on where/how the class could be safely used. So some discussion seemed warranted. Would Trevor's recent unique_ptr bits simplify things here? Jeff