Re: [PATCH 1/3] c-family: add name_hint/deferred_diagnostic

[Jeff Law <law at redhat dot com>]

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