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

[David Malcolm <dmalcolm at redhat dot com>]

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.

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?

Do you have any preferences?

> 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.

Thanks for the review so far.
Dave