Re: [PATCH]: __attribute__((deprecated)) (revision2)

[Ira Ruben <ira at apple dot com>]

At 6:16 PM -0800 1/7/02, Richard Henderson wrote:

>On Mon, Dec 17, 2001 at 05:29:10PM -0800, Ira Ruben wrote:
>> +   { "deprecated",             0, 0, false, false, false,
>> +			      handle_deprecated_attribute },
>
>Why isn't decl_required true?  Seems like it only makes sense
>to deprecate symbols.  Perhaps there's some queer interaction
>with typedefs that I'm not seeing right away?  Though I'd
>really expect a TYPE_DECL to show up here in the typedef case.

I used handle_unused_attribute() as a guide for my code.  Since the
decl_required wasn't set for it I assumed I shouldn't set it for
mine either.  I also think decl_required would mean that only
nodes where DECL_P is true are allowed.  I am also handling
cases where TYPE_P is true (unused does also).


> > +       if (TREE_CODE (decl) == TYPE_DECL)
>> +      	TREE_DEPRECATED (decl) = 1;
>> +       else if (TREE_CODE (decl) == PARM_DECL
>> +	  || TREE_CODE (decl) == VAR_DECL
>> +	  || TREE_CODE (decl) == FUNCTION_DECL
> > +	  || TREE_CODE (decl) == FIELD_DECL)
>> +	TREE_DEPRECATED (decl) = 1;
>
>Why are these cases distinguished?

I think what happened was that while the code was under development
I had some different stuff going on with TYPE_DECL's.  It eventually
evolved to what I have but in the editing I didn't notice that the
TYPE_DECL had degenerated to the same as the other cases.  I can
combine them.


> > +   for (a = attributes; a; a = TREE_CHAIN (a))
>> +     {
>> +       tree name = TREE_PURPOSE (a);
>> +       if (TREE_CODE (name) == IDENTIFIER_NODE)
> > +        if (strcmp (IDENTIFIER_POINTER (name), "deprecated") == 0)
>
>Use lookup_attribute.

Three reason's I didn't:

1. I was concerned about the overhead I was imposing on decls (which
   I suppose you could argue that how much overhead could there
   be since how many attributes could there be).
2. I have in my sources additional code for a future patch related
   to this stuff which requires an additional strncmp.
3. I didn't know about lookup_attribute :-)  But it is somewhat
   more expensive and would eventually require me to do two passes
   over the attributes to look up that 2nd keyword.  That or call
   is_attribute_p instead of using lookup_attribute.

I guess I can change it to lookup_attribute() unless you don't have
any objections to the is_attribute_p explicit call.


> > +       /* If the entire declaration is itself tagged as deprecated then
>> +          suppress reports of deprecated items.  */
> > +       if (id && TREE_DEPRECATED (id))
> > +         {
> > +         if (deprecated_state != DEPRECATED_SUPPRESS)
>
>Combine the two conditions.

As I mentioned above, there will eventually be a follow-on patch related
to this.  The body of that if will be an else part of an inner if.  I
prefer to keep it this way.  But I won't argue it too much if you
insist.


>
>>      tree *f;
> > +   extern int adding_implicit_members;
>
>This is considered Very Bad Style.  Include the proper header.

I don't want to argue with you about "style".  This is referenced in only
cp/decl.c and set only in class.c.  It was the last fix to the code and
I didn't like doing it since it was a hack so that decl could know it's
declaring implicit members.  But there is no simpler technique.  There
IS precedent in the compiler for explicit externs (admittedly not many).
Here's few from a quick search:

c-decl.c: flag_keep_static_consts
final.c: count_instrumented_edges
final.c: length_unit_log
toplev.c:size_directive_output
toplev.c:last_assemble_variable_decl
toplev.c: target_flags

At any rate I'll put the extern in cp/cp-tree.h with all the others
that are there.

Thanks for the feedback.

Should I "bounce" the other patch (attributes on inline members) off
Mark Mitchell?

Ira