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 <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:
That would be one way, though you can avoid allocating on the heap by
doing something like this.
std::option<name_hint> hint;
if (something)
foo (&something);
void
foo(std::option<name_hint> *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
