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
