On Wed, Sep 5, 2012 at 3:47 PM, Jan Hubicka <hubi...@ucw.cz> wrote:
>> What do you think of the following plan for turning cgraph into
>> a class hierarchy? We cannot finish it until we have gengtype
>> understanding single inheritance, but we can start changing APIs
>> in preparation.
>
> Good you told me, I was about trying that myself. Did not know gengtype
> do not understand inheritance yet.
>>
>> APPROACH ###########################################
>>
>> Add converters and testers.
>> Change callers to use those.
>> Change callers to use type-safe parameters.
>> Change implementation to class hierarchy.
>> Add accessors.
>
> Sounds good to me.
>>
>>
>> CONVERTERS AND TESTERS ###########################################
>>
>> add
>> symtab_node_base &symtab_node_def::ref_symbol()
>> { return symbol; }
>> symtab_node_base &cgraph_node::ref_symbol()
>> { return symbol; }
>> symtab_node_base &varpool_node::ref_symbol()
>> { return symbol; }
>>
>> change
>> node->symbol.whatever
>> to
>> node->ref_symbol().whatever
>
> OK, the basic idea is that symtab_node is basetype of cgraph_node and
> varpool_node. We may want to drop the historica cgraph/varpool names here,
> since function_node/variable_node would sound better. Cgraph still exists, but
> varpool is more or less historical relic.
The cgraph redesign probably deserves more discussion.
1) It may be worthwhile to abstract the graph manipulation code into a
utility class which is templatized.
graph<T>, node<T> with node inheriting from T.
2) Introduce a global symbol table containing a function table and a
global variable table. The function table should replace the current
cgraph node link list, and the variable table replaces the varpool.
The symbol table should provide basic interfaces to do named based
lookup, traversal, alias handling etc. I noticed trunk already has
some of that -- but it seems more abstraction is needed.
3) it seems natural to drop 'node' in the naming scheme
symbol (symtab_entry) --> base class;
function --> derived from symbol; (It conflicts with the existing
struct function though);
variable --> derived from symbol;
typedef node<function> cnode;
4) coding convention is needed for functions that do 'casting' and
'trial casting'.
thanks,
David
>
> I would expect inheritance to allow me to write node->whatever and to have
> casting operators to convert things back and forth, so we only need to add
> testers? Probably is_function/is_variable & try_function/try_variable sounds
> more readable to me.
> What do you think? (I just arrived from China, so will take it more tought
> once unjetlagged)
>
> Honza
>>
>> ----
>>
>> should not need to add these
>>
>> cgraph_node &symtab_node_def::ref_cgraph()
>> { gcc_assert (symbol.type == SYMTAB_FUNCTION);
>> return x_function; }
>> varpool_node &symtab_node_def::ref_varpool()
>> { gcc_assert (symbol.type == SYMTAB_VARIABLE);
>> return x_variable; }
>>
>> ----
>>
>> add
>> symtab_node_base *symtab_node_def::try_symbol()
>> { return &symbol; }
>> cgraph_node *symtab_node_def::try_cgraph()
>> { return symbol.type == SYMTAB_FUNCTION ? &x_function : NULL; }
>> varpool_node *symtab_node_def::try_varpool()
>> { return symbol.type == SYMTAB_VARIABLE ? &x_variable : NULL; }
>>
>> change
>> if (symtab_function_p (node) && cgraph (node)->analyzed)
>> return cgraph (node);
>> to
>> if (cgraph_node *p = node->try_cgraph())
>> if (p->analyzed)
>> return p;
>>
>> change
>> if (symtab_function_p (node) && cgraph (node)->callers)
>> ....
>> to
>> if (cgraph_node *p = node->try_cgraph())
>> if (p->callers)
>> ....
>>
>> change
>> if (symtab_function_p (node))
>> {
>> struct cgraph_node *cnode = cgraph (node);
>> ....
>> to
>> if (cgraph_node *cnode = node->try_cgraph ())
>> {
>> ....
>>
>>
>> likewise "symtab_variable_p (node)" and "varpool (node)"
>>
>> ----
>>
>> If there are any "symtab_function_p (node)" expressions left,
>>
>> add
>> bool symtab_node_def::is_cgraph()
>> { return symbol.type == SYMTAB_FUNCTION; }
>> bool symtab_node_def::is_varpool()
>> { return symbol.type == SYMTAB_VARIABLE; }
>>
>> change
>> symtab_function_p (node)
>> to
>> node->is_cgraph ()
>>
>> likewise "symtab_variable_p (node)"
>>
>>
>> ----
>>
>> Though we would like to avoid doing so,
>> if there are any "cgraph (node)" or "varpool (node)" expressions left,
>>
>> add
>>
>> symtab_node_base *symtab_node_def::ptr_symbol()
>> { return &symbol; }
>> cgraph_node *symtab_node_def::ptr_cgraph()
>> { gcc_assert (symbol.type == SYMTAB_FUNCTION);
>> { return &x_function; }
>> varpool_node *symtab_node_def::ptr_varpool()
>> { gcc_assert (symbol.type == SYMTAB_VARIABLE);
>> { return &x_variable; }
>>
>> change
>> cgraph (node) => node->ptr_cgraph()
>>
>> likewise "varpool (node)"
>>
>>
>> TYPE SAFETY ###########################################
>>
>> If a function asserts that its symtab_node parameter is symtab_function_p,
>> then convert the function to take a cgraph_node*
>> and change the callers to convert as above.
>>
>>
>> PROPOSED HIERARCHY ###########################################
>>
>> enum symtab_type { SYMTAB_SYMBOL, SYMTAB_FUNCTION, SYMTAB_VARIABLE };
>>
>> struct symtab_node_base /* marked SYMTAB_SYMBOL */
>> embeds symtab_type
>>
>> cgraph_node : symtab_node_base /* marked SYMTAB_FUNCTION */
>>
>> varpool_node : symtab_node_base /* marked SYMTAB_VARIABLE */
>>
>> changing
>> typedef union symtab_node_def *symtab_node;
>> typedef const union symtab_node_def *const_symtab_node;
>> to
>> typedef symtab_node_base *symtab_node;
>> typedef const symtab_node_base *const_symtab_node;
>>
>> changing used of
>> symtab_node_def
>> to
>> symtab_node_base
>>
>> --
>> Lawrence Crowl
