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

[Jan Hubicka <hubicka at ucw dot cz>]

Hi,
sorry for late reply, I noticed the patch only yesterday.

> Honza: following up from our IRC chat, I've ported the symtab, cgraph
> and varpool nodes from the current hand-coded inheritance-in-C scheme
> to being a C+ class hierarchy.
> 
> I know you're in the middle of making lots of other changes to this code,
> so I've written a script to automate the parts of the conversion where
> appropriate - see below.
> 
> 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

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

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
>     This is needed to avoid generating a malfunctioning gtype-desc.c
>     *some* of the time - in capriciously changing ways as the sources
>     change... :(   I sent it separately since I'm running into this
>     issue with other changes I'm trying out; it "randomly" appears
>     when trying to use GTY((user)).

I can't approve this change, unfortunately.
Note that cgraph node also go into PCH, so be sure to check that PCH
works well with the changes.
> 
>   * Patch 1 of the 2 in this series is the hand-written part of the
>     conversion.  This makes the changes above, plus various workarounds
>     for dealing with yet more issues in how gengtype handles
>     GTY((user)).

I will comment on this in the followup mail.
> 
>   * 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