Re: [RFA] Emit DWARF for template parameters (PR debug/30161)

[Jason Merrill <jason at redhat dot com>]

On 08/13/2009 03:12 PM, Dodji Seketeli wrote:
> +/* Return true if T is a primary function
> +   template instantiation.  */
> +
> +bool
> +decl_template_instantiation_p (const_tree t)

The name should have "primary" in it if that's a condition.

> +    case DW_AT_GNU_name:
> +      return "DW_AT_GNU_name";

This name isn't specific enough.  Perhaps DW_AT_GNU_template_name or 
_value_name or _name_value or _string_value or something.

> +      || (!lang_hooks.types.generic_p (t)
> +          && !lang_hooks.decls.generic_p (t)))
> +    return;
> +
> +  if (TYPE_P (t))
> +    die = lookup_type_die (t);
> +  else if (DECL_P (t))
> +    die = lookup_decl_die (t);
> +
> +  gcc_assert (die);
> +
> +  parms = lang_hooks.types.get_innermost_generic_parms (t);

Why have two different generic_p hooks, but only one get_parms hook? 
Probably better to have just one hook on both cases.  Or even do away 
with both generic_p hooks and just have the get_parms hook return null 
for non-generics.

> +  for (i = 0; i < parms_num; i++)
> +    {
> +      tree args = lang_hooks.types.get_innermost_generic_args (t);

Why re-fetch the args each time we go through the loop?

> +	  if (lang_hooks.types.generic_type_argument_pack_p (arg)
> +	      || lang_hooks.types.generic_nontype_argument_pack_p (arg))
> +	    {
> +	      int i = 0, len = 0;
> +	      tree pack_elems =
> +		lang_hooks.types.get_argument_pack_elems (arg);

Again, why two hooks to test but only one to fetch?  And again, I don't 
think we even need a separate test hook at all.  Just try to get the 
pack elements, and return null if it isn't a pack (as the hook 
implementation already does).

> +  else if (lang_hooks.decls.generic_type_parameter_decl_p (parm))

Why not just test TREE_CODE (parm) == TYPE_DECL?

> +  if (TREE_CODE (parm) == PARM_DECL || TREE_CODE (parm) == CONST_DECL)
> +    /* PARM is a nontype generic parameter  */
...
> +      if (TREE_CODE (parm) == PARM_DECL)
> +	{
> +	  /* So PARM is a non-type generic parameter.

Why are these tests different?

> -tree_add_const_value_attribute (dw_die_ref var_die, tree decl)
> +tree_add_const_value_attribute (dw_die_ref var_die, tree t)

Currently this function takes as its second argument the decl whose 
constant value we want to add to the die (which is the die for that same 
decl).  You're changing it to either take that decl OR the constant 
value itself, which is going to cause trouble.  I think it would be 
better to change it to only take the constant value, and move the code 
for deciding which types of decls to bother with into a separate 
function like tree_add_const_value_attribute_for_decl.

Why do you need the other code changes?  Why can't you just use 
rtl_for_decl_init for template arguments, too?

> +  /* Returns the name of the ith element of a generic parameter pack.  */
> +  tree (*make_ith_generic_pack_parm_name) (tree, int);

I don't think this needs to be a hook; there's no need for the front end 
internal name to match the name in the debug info.  In this case, i 
think it's better to duplicate the C++ front end function in dwarf2out.c.

> -  /* True if the type is an instantiation of a generic type,
> -     e.g. C++ template implicit specializations.  */
> +  /* True if the parameter is an instantiation of a generic type,
> +     e.g. C++ class template specializations.  */

The change to "parameter" is wrong.

> +  /* Retuns the generic parameters of a generic declaration,
> +     e.g. C++ template declaration.  */
> +  tree (*get_innermost_generic_parms) (const_tree);

The argument isn't a template, it's an instantiation.

> +// We don't support debug info output for template template parameter yet.
> +// Look at the comment near the end of the test source code for more.

This comment seems out of date.  :)

Jason