Re: [PATCH 0/2] Port symtab/cgraph/varpool nodes to use C++ inheritance

[David Malcolm <dmalcolm at redhat dot com>]

On Tue, 2013-08-20 at 23:01 +0200, Jan Hubicka wrote:
[...]
> > 
> > In summary,
> >   struct GTY(()) symtab_node_base
> > becomes:
> >   class GTY((user)) symtab_node_base
> > 
> > and the subclasses:
> >   struct GTY(()) cgraph_node
> > and:
> >   struct GTY(()) varpool_node
> > 
> > become (respectively):
> >   struct GTY((user)) cgraph_node : public symtab_node_base
> > and:
> >   class GTY((user)) varpool_node : public symtab_node_base
> 
> I am not quite sure why we need symtab_node_base.
> 
> symtab_node is meant to be basetype of the symbols.  Symbols can be either functions or variables.
> I would expect then
>    struct GTY((user)) cgraph_node : public symtab_node
>    class GTY((user)) varpool_node : public symtab_node

Indeed, I would have used "symtab_node", but it's already in use as the
typedef of the *pointer* type - to quote the current ipa-ref.h:

   typedef union symtab_node_def *symtab_node;

So I kept the name "symtab_node_base".

I was also holding off on renames since you have other changes
in-flight.

> > The symtab_node_def union goes away, as do the "symbol" fields at the
> > top of the two subclasses.
> 
> Yes, this is very nice.
> > 
> > I kept the existing names for things, and the "struct" for cgraph_node
> > since it is often referred to as "struct cgraph_node".
> 
> In fact I think we should take the chance to rename
> cgraph_node -> symtab_function
> varpool_node-> symtab_variable
> (or symtab_func/symtab_var as suggested by Martin today, I am fine with both)
> symtab_node may also be better as symtab_symbol.  So the hearchy would be
> that symbol table consists of symbols and symbols can be function or entries.
> 
> The original naming comes from callgraph code that is over decade old now.
> node is because graph has nodes and edges.
> 
> We can also do this incrementally on the top of the hard work of getting the
> hiearchy in at first place, if that seems easier for you.  Indeed there are
> some references to it in comments, but they should be updated.

Given how much of this is done by a script (with its own test suite),
I'd be up for getting in such other changes at the same time.

Let the wild rumpus^Wbikesheddery commence! 

How about:

  class symtab_sym {};
  class symtab_func : public symtab_sym {};
  class symtab_var : public symtab_sym {};
  typedef symtab_sym *symtab_node;

In an ideal world I'd use a namespace for this, but I'm wary about
confusions between the symtab meaning of "function" vs the cfun meaning
of "function".

> I am OK with temporary inconsistency with naming and I will be happy to
> continue with incremental cleanups.
> > 
> > The patch is in three parts; all three are needed.
> > 
> >   * a fix for a bug in gengtype:
> >       http://gcc.gnu.org/ml/gcc-patches/2013-08/msg00882.html
[...]
> I can't approve this change, unfortunately.

This one is already approved and committed.

> >   * Patch 1 of the 2 in this series is the hand-written part of the
[...]

> > 
> >   * Patch 2 of the 2 in this series is the autogenerated part.
> >     Currently to access the base symtab fields of a cgraph or varpool
> >     node, the code has e.g.
> > 
> >        node->symbol.decl
> > 
> >     whereas with C++ inheritance, the "symbol" field is no more, and we
> >     directly use the base-class field:
> > 
> >        node->decl
> 
> Indeed, this is very nice.  We also use
> (symtab_node)node whenver we need to go from cgraph/varpool node back
> to basetype.  These should go, too.
> Finally I introduced cgraph(node)/varpool(node) functions that converts
> symtab node to cgraph/varpool node and ICEs when failed.
> 
> We probably should use our new template conversions.  We have is_a
> predicate and dyn_cast convertor that returns NULL on failure.  Do
> we have variant that ICEs when conversion is not possible?
> > 
> > How does this look?  Are there other automated changes you'd like me
> > to make?   How should I go about getting this into trunk, given that you
> > have many other changes to this code on the way?
> 
> I like the general direction and I am grateful you work on it :) Lets go with
> this as first step.  Incrementally I think the ipa-ref API should be cleaned up
> (it worked around model before we had basetype for cgraph/varpool) and I am
> having quite long TODO list on the top of that.
> 
> Honza