Re: [PATCH] C++/DWARF : Add 'using' support - take 2

[Jason Merrill <jason at redhat dot com>]

On Tue, 13 Jan 2004 13:54:46 -0800, Devang Patel <dpatel@apple.com> wrote:

> On Jan 13, 2004, at 1:06 PM, Jason Merrill wrote:

>> These hunks seem useless; you aren't using tmp_die anywhere.  You really
>> ought to review your own patches for this sort of thing before you ask
>> someone else to review them.
>
> I intentionally added it to note that now gen_subprogram_die returns new
> die.  If you want I will remove it.

Please.

>> I'd prefer to do without all the changes to make gen_subprogram_die return
>> a value.  I don't see a good reason to break the convention of "generate,
>> then look up" for just this one variety of DIE.
>
> We do not equate abstract dies with decl number

Huh?

  else if (DECL_ABSTRACT (decl))
    { 
...
      equate_decl_number_to_die (decl, subr_die);
    }

>> You should handle this in the "else if (old_die)" block below, which also
>> deals with reusing a die.
>
> It handles reusing decl die as definition die. We need to complete this
> die which is the else section after "else if (old_die)".

What do you mean by "complete"?  The die created by force_decl_die should
be a declaration die, so reusing it as a definition die should be what we
want.

>> I don't see how checking die_definition makes any sense here.  What
>> situation is this code trying to handle?
>
> If variable for forced out earlier than later on add_AT_specification()
> aborts because die_definition is non-zero.

Why is it non-zero?

>>> ! force_decl_die (tree decl)
>>> ! {
>>> ...
>>> ! 	  else if (DECL_INLINE (decl))
>>> ! 	    /* Force die to represent this function as abstract
>>> ! 	       function for our reference.  */
>>> ! 	      dwarf2out_abstract_function (decl);
>>
>> I realize that I suggested this in my earlier review, but I think we
>> shouldn't handle inline functions differently here.  If all we care about
>> is having a DIE to refer to, we don't need to emit the whole abstract
>> function.
>
> so instead what do you suggest here?

I think we want to force a declaration die in all cases, regardless of
whether or not the function has been defined.  I'm reluctant to change the
order in which definition dies are emitted.

>>  (btw, checking DECL_INLINE is the wrong way to test this)
>
> why?

Because DECL_INLINE doesn't tell you whether or not we want an abstract
instance of a particular function.  cgraph_function_possibly_inlined_p is
better, but still misses cases where we use abstract instances in the
absence of inlining.  Cloning, for instance.

>>> ! 	case FIELD_DECL:
>>> ! 	case VAR_DECL:
>>> ! 	  gen_decl_die (decl, context_die);
>>> ! 	  break;
>>> ! 	default:
>>> ! 	  dwarf2out_decl (decl);
>>> ! 	  break;
>>
>> What other decls are likely to come up?  Why is dwarf2out_decl the right
>> thing for them?
>
> I do not remember. May be it is here because force_decl_die() is a
> replacement for original dwarf2out_decl() call I had. I'll test again
> by adding abort() for other decls.

Thanks.

>>> *************** do_using_directive (tree namespace)

>> Also, this is wrong for function local using-directives; they need to be
>> in current_scope(), not current_namespace.
>
> I use current_scope() in do_local_using_decl().

And you should use it here, too; that deals with using-declaration, this
deals with using-directives.

Jason