Your 'Class <Protocol>' Work

Ziemowit Laski zlaski@apple.com
Wed Sep 29 21:25:00 GMT 2004


On 29 Sep 2004, at 10.34, David Ayers wrote:

> Ziemowit Laski wrote:
>
>> David,
>>
>> Aside from a general plea to clean up your formatting/spacing :-), I
>> have a few questions/comments.
>
> Hello Zem,
>
> Could you be a bit more specific on how to improve formatting spacing?
> I've tried to follow the documented conventions and looking at:
>
> http://gcc.gnu.org/ml/gcc-patches/2004-09/msg02888.html
>
> I don't think my mailer (mozilla 1.7) has mangled anything 
> unexpectedly.
>  But maybe I just have some fundamental misunderstanding wrt to the
> formating guidelines.

You are missing a space following opening parenths in several places, 
e.g.,

>>> +	{
>>> +	  add_method_to_hash_list(cls_method_hash_list,  method_decl);
>>> +	}

where you also have an extraneous space (or perhaps a tab?) after the 
comma.

>
>>> objc_finish_interface (void)
>>> {
>>>   finish_class (objc_interface_context);
>>> +
>>> +  if (TREE_CODE (objc_interface_context) == PROTOCOL_INTERFACE_TYPE
>>> +      && PROTOCOL_DECLARED_BY_ROOT (objc_interface_context))
>>
>>
>> This threw me off earlier, so perhaps you could rename
>> PROTOCOL_DECLARED_BY_ROOT to
>> PROTOCOL_USED_IN_ROOT_CLASS or something along those lines?
>
> Sure.  How about PROTOCOL_ADOPTED_BY_ROOT_CLASS?

Sounds good. :-)
>
>>> +    {
>>> +      tree protocol = objc_interface_context;
>>> +      tree method_decl;
>>> +
>>> +      /* Since we do not have a protocol list,
>>> +	 go ahead and register the method list directly.  */
>>> +      for (method_decl = PROTOCOL_NST_METHODS (protocol);
>>> +	   method_decl;
>>> +	   method_decl = TREE_CHAIN (method_decl))
>>> +	{
>>> +	  add_method_to_hash_list(cls_method_hash_list,  method_decl);
>>> +	}
>>
>>
>> I suspect this is overkill, since instance methods of root classes are
>> already added to cls_method_hash_list,
>> are they not?
>
> Well, yes, instance methods of root classes are already in
> cls_method_hash_list but generally/often the methods of adopted
> protocols are not repeated in the interface declaration by convention,
> which is what makes this necessary, I believe.

Ok, that's a good point.  I guess there's still overkill in that the 
instance
methods thusly added will serve as a fallback for _any_ class method 
lookup
(i.e., even if the protocol in question is not involved), but the same 
overkill
happens wrt root classes, which also happily stuff all of their 
instance methods
into cls_method_hash_list.  So yes, I think your approach is consistent.

However, this raises a different question in my mind: If protocols 
adopted by
root classes already stuff their instance methods into the global class 
method
table, do we need to do anything more than we already do to find them?  
In
other words, if I have

       @protocol Proto2
       - someMessage;
       @end

       @interface Root <Proto2>
       @end

       Class <Proto1, Proto2> obj;
         :
       [obj someMessage];

the compiler should look for +someMessage in Proto1 and Proto2 (only a 
slight
variation from the 'id <Proto1, Proto2>' case, where both -someMessage 
and
+someMessage would be sought) and then fall back to the the global class
method table (again, a slight variation from 'id <Proto1, Proto2>' where
both global tables would be examined), and therein find the -someMessage
method from Proto2.

What do you think?  Or am I missing something?

>
>>> +
>>> +      /* This protocol is marked.  Now insure that that all instance
>>
>>
>> s/insure/ensure/ :-), although you probably just want to say that
>> protocols inherited by a marked protocols are
>> themselves marked.
>
> Actually what I meant was:
>
>       /* This protocol is marked.  Now ensure that that all instance
>          prototypes of incorporated protocols are registered as class
>          methods without marking the protocols.  */

Perhaps instead of 'incorporated', we should consistently say 
'inherited'?
What do Java people say when one interface refers to another? :-)

>
> ... but ... hmm ...
>
>>> +	 prototypes of inherited protocols get registered as class
>>> +	 methods without marking them.  */
>>> +      mark_protocols_declared_by_root_class (PROTOCOL_LIST
>>> (protocol), 1);
>>> +    }
>>> +
>>
>>
>> This also seems like a good time to handle root class (and category?)
>> interfaces and mark the protocols they inherit.

Now I have to correct myself: s/inherit/adopt. :-)

>
> Agreed, this lets us keep them in one place.
>
>>
>>>   objc_interface_context = NULL_TREE;
>>> }
>>
>>
>>> @@ -836,6 +860,84 @@
>>> }
>>>
>>> /* Return 1 if LHS and RHS are compatible types for assignment or
>>> +   various other operations.  Return 0 if they are incompatible.
>>> +   When the operation is REFLEXIVE (typically comparisons), check
>>
>>
>> s/REFLEXIVE/SYMMETRIC/; objc_comptypes() suffers from the same
>> misnomer, it
>> just never seemed important enough to fix. :-)
>
> Can I offer a follow up patch to deal with this and more "pressing"
> naming issues in objc_comptypes?  Take a look at (patched) line 1057:
>
>               if (reflexive)
>                 {
>                   tree rname = OBJC_TYPE_NAME (TREE_TYPE (lhs));
>                   tree rinter;
>                   tree rproto, rproto_list = TYPE_PROTOCOL_LIST (rhs);
>
> where we use rname and later rinter for the lhs trees.

>
>> Actually, you probably won't
>> need objc_comptypes_proto_proto() after all (see below), but if you
>> could s/reflexive/symmetric/ in objc_comptypes(), that would be great.
>
> I think it be better to do this in a separate patch.  But if you think
> the cleanup is appropriate for stage 3, I offer a follow up patch
> dealing with these two and any other issues I find.

Sure, let's deal with it later.  I see no reason why such cleanup would
not be appropriate for Stage 3, though folks above me on the totem pole
may obviously overrule me here if they wish. :-)

Actually, I do think objc_comptypes() needs to be seriously reworked
to remove the spaghetti, but that will have to wait for gcc 4.1 stage 
1...
>
>>> +/* Return 1 if LHS and RHS are compatible types for assignment or
>>>    various other operations.  Return 0 if they are incompatible, and
>>>    return -1 if we choose to not decide (because the types are really
>>>    just C types, not ObjC specific ones).  When the operation is
>>> @@ -866,76 +968,21 @@
>>>       && TREE_CODE (rhs) == POINTER_TYPE
>>>       && TREE_CODE (TREE_TYPE (rhs)) == RECORD_TYPE)
>>>     {
>>> -      int lhs_is_proto = IS_PROTOCOL_QUALIFIED_ID (lhs);
>>> -      int rhs_is_proto = IS_PROTOCOL_QUALIFIED_ID (rhs);
>>> +      int lhs_is_proto_id = IS_PROTOCOL_QUALIFIED_ID (lhs);
>>> +      int rhs_is_proto_id = IS_PROTOCOL_QUALIFIED_ID (rhs);
>>> +      int lhs_is_proto_class = IS_PROTOCOL_QUALIFIED_CLASS (lhs);
>>> +      int rhs_is_proto_class = IS_PROTOCOL_QUALIFIED_CLASS (rhs);
>>
>>
>> I'm not sure why it is necessary to distinguish between
>> IS_PROTOCOL_QUALIFIED_ID
>> and IS_PROTOCOL_QUALIFIED_CLASS for purposes for objc_comptypes().  In
>> most
>> cases, the two behave identically for type comparison purposes, and in
>> any places
>> they do not you can use IS_ID / IS_CLASS to distinguish them.  Then,
>> you can
>> keep objc_comptypes() mostly as it is, and get rid of
>> objc_comptypes_proto_proto().
>
> I just tried to stay in line with the existing implementation.
> Actually, I'm not sure what value that macro has.  It is used only here
> in comptypes and as you now need to work with IS_ID/IS_CLASS anyway we
> might as well test with PROTOCOL_LIST.  So in this patch I've removed
> the existing macro.
>
> I'm not sure whether I can sensibly simplify objc_comptypes to get
> rid of objc_comptypes_proto_proto, at least not without a considerable
> rewrite of objc_comptypes which I'd rather not do at this stage.  But 
> if
> it's really important to you, I'll look into it.

Hmm... I guess my comment was motivated by the belief that 'Class 
<Proto>'
is virtually identical to 'id <Proto>' as far as type comparisons are
concerned, and the few differences (if any) could easily be 
special-cased.
Do you disagree?

Perhaps creating a routine like objc_comptypes_proto_proto() would make
sense as part of a deeper objc_comptypes() rewrite that I've alluded to
before, but right now this strikes me as unduly invasive.  For the same
reason, I would favor keeping (though possibly renaming, if you like)
the IS_PROTOCOL_QUALIFIED_ID macro and enhancing it to accept 'Class 
<Proto>'.
Longer term, perhaps killing this macro altogether is a good idea, as
you suggest.

>
>>> @@ -1164,7 +1251,7 @@
>>>
>>> /* Construct a PROTOCOLS-qualified variant of INTERFACE, where
>>> INTERFACE may
>>>    either name an Objective-C class, or refer to the special 'id' or
>>> 'Class'
>>> -   types.  If INTERFACE is not a valid ObjC type, just return it
>>> unchanged.  */
>>> +   types.  If INTERFACE is not a valid ObjC type, report error.  */
>>>
>>> tree
>>> objc_get_protocol_qualified_type (tree interface, tree protocols)
>>> @@ -1180,7 +1267,7 @@
>>>       if (type)
>>> 	type = xref_tag (RECORD_TYPE, type);
>>>       else
>>> -        return interface;
>>> +        error ("protocol qualifiers specified for non-Objective-C
>>> type");
>>
>>
>> Thanks for the error message, but you should still return here instead
>> of falling through, I think...
>
> Indeed!  In fact I think we should return an error_mark_node.
>
>>> +      /* Minor efficency check.  Expect all protocols which have
>>> +	 been previously marked, to have had their methods registered.
>>
>>
>> s/efficency/efficiency/.
>
> Thanks.
>
>>> +	 A protocol which itself is not declared by a root class yet
>>> +	 is inherited by multiple root classes will have its instance
>>
>>
>> What is the difference between "declared by a root class" and
>> "inherited by
>> a root class"?  I was assuming the two are synonymous...
>
> Sorry, bad terminology, bad wording.  It should have read:
>
>       /* Minor efficiency check.  Expect all protocols which have
>          been previously marked, to have had their methods registered.
>          A protocol which itself is not *adopted* by a root class, yet
>          is *incorporated* by other protocols which are adopted by root
>          classes will have its instance methods processed redundantly,
>          but it does not seem worth while to flag them just to avoid
>          this.  */
>
> ... but hmm again ...
>
>>> +	 methods processed redundantly, but it does not seem worth while
>>> +	 to flag them just to avoid this.  */
>>> +      if (!PROTOCOL_DECLARED_BY_ROOT (protocol))
>>> +	{
>>> +	  tree method_decl;
>>> +
>>> +	  for (method_decl = PROTOCOL_NST_METHODS (protocol);
>>> +	       method_decl;
>>> +	       method_decl = TREE_CHAIN (method_decl))
>>> +	    {
>>> +	      add_method_to_hash_list(cls_method_hash_list,  method_decl);
>>> +	    }
>>> +	}
>>
>>
>> Again, I suspect that adding these to cls_method_hash_list is 
>> redundant
>> (see above).
>>
>
> I don't, see above :-).

Agreed. :-)
>
>>> +/* Searches for SELECTOR in the instance methods
>>> +   of the protocol interface list RPROTOS.  Returns
>>> +   the prototype only if the corresponding protocol from RPROTOS
>>> +   is declared by any root class.  IGNORE should always be 0.
>>> +   It is used for the recursive prototype search of inherited
>>> protocols.  */
>>> +
>>> +static tree
>>> +instance_prototype_of_root_protocol (tree rprotos, tree selector, 
>>> int
>>> ignore)
> ...
>>> @@ -5569,14 +5745,24 @@
>>> 	}
>>>       else
>>> 	{
>>> +	  rprotos = TYPE_PROTOCOL_LIST (rtype);
>>> 	  class_tree = objc_class_name;
>>> 	  OBJC_SET_TYPE_NAME (rtype, class_tree);
>>> +
>>> +	  if (rprotos)
>>> +	    rtype = NULL_TREE;
>>> 	}
>>>
>>>       if (rprotos)
>>> -	method_prototype
>>> -	  = lookup_method_in_protocol_list (rprotos, sel_name,
>>> -					    class_tree != NULL_TREE);
>>> +	{
>>> +	  method_prototype
>>> +	    = lookup_method_in_protocol_list (rprotos, sel_name,
>>> +					      class_tree != NULL_TREE);
>>> +	  if (!method_prototype && class_tree != NULL_TREE)
>>> +	    method_prototype
>>> +	      = instance_prototype_of_root_protocol(rprotos, sel_name, 0);
>>> +	}
>>> +
>>
>>
>> Here, instead of calling (and defining!)
>> instance_prototype_of_root_protocol(), perhaps you
>> can simply tweak lookup_method_in_protocol_list() so that if a class
>> method is sought,
>> it can fall back to instance methods for protocols used in root 
>> classes?
>
> I think so.  I may have to add a parameter to handle the recursive
> search with respect to incorporated protocols, but ...hmm... maybe I 
> don't.

Actually, if my hunch above is correct (and I hope it is :-), you 
shouldn't need to modify
lookup_method_in_protocol_list() at all, and you will not need 
instance_prototype_of_root_protocol()
either since lookup_method_in_hash_lists() should find the instance 
methods for you.
Hopefully, the sole use of the PROTOCOL_ADOPTED_BY_ROOT_CLASS flag will 
henceforth be to
indicate that a protocol's instance methods were put into the global 
class method hash table.
>
>>> Index: gcc/objc/objc-act.h
>>> ===================================================================
>>> RCS file: /cvsroot/gcc/gcc/gcc/objc/objc-act.h,v
>>> retrieving revision 1.31
>>> diff -u -r1.31 objc-act.h
>>> --- gcc/objc/objc-act.h	22 Sep 2004 01:13:07 -0000	1.31
>>> +++ gcc/objc/objc-act.h	28 Sep 2004 18:23:08 -0000
> ...
>>> @@ -279,6 +288,8 @@
>>>   (POINTER_TYPE_P (TYPE) && TREE_TYPE (TYPE) == TREE_TYPE
>>> (objc_class_type))
>>> #define IS_PROTOCOL_QUALIFIED_ID(TYPE) \
>>>   (IS_ID (TYPE) && TYPE_PROTOCOL_LIST (TYPE))
>>> +#define IS_PROTOCOL_QUALIFIED_CLASS(TYPE) \
>>> +  (IS_CLASS (TYPE) && TYPE_PROTOCOL_LIST (TYPE))
>>
>>
>> As I mentioned above, instead of adding IS_PROTOCOL_QUALIFIED_CLASS,
>> I'd simply enhance IS_PROTOCOL_QUALIFIED_ID thusly:
>>
>>     #define IS_PROTOCOL_QUALIFIED_ID(TYPE) \
>>        ((IS_ID (TYPE) || IS_CLASS (TYPE)) && TYPE_PROTOCOL_LIST 
>> (TYPE))
>>
>> This should simplify the objc_comptypes() logic greatly.  Feel free to
>> rename this macro to IS_PROTO_QUAL_ID_OR_CLASS
>> or some such if you'd like...
>
> Like I implied above, I don't think rewriting objc_comptypes at this
> stage is a good idea, but if that is a prerequisite, I'll try.

Actually, my objection was that you perturbed objc_comptypes() _too 
much_. :-)
I absolutely agree that this is not the time for rewriting it.

> Now to my three '...hmm...'s:  I had originally decided it was a bad
> idea to mark protocols, that are incorporated by protocols which were
> adopted by root classes, as also being adopted by root classes.  The
> issue is in the test case.  Consider:
>
> @protocol MyProto8
> +(void)doItClass8;
> -(void)doItInstance8;
> @end
>
> @protocol MyProto9 <MyProto8>
> +(void)doItClass9;
> -(void)doItInstance9;
> @end
>
> @interface MyClass1 <MyProto9>
> @end
>
> Class <MyProto8> clsP8 = 0;
>
> ...
>   [clsP8 doItInstance8]; /* { dg-warning "not implemented" } */
>
> Now, does the fact that a root class adopts MyProto9, which 
> incorporates
> MyProto8, make MyProto8 a "root" protocol in the sense that we should
> search for instance methods when sending messages to Class <Proto8> ?  
> I
> admit this is a corner case that may not warrant the extra complexity.
> This updated patch would make MyProto8 a full fledged "root" protocol.

I agree; since MyProto9 is a "root protocol", then so is MyProto8, and 
so
the warning above should not be generated.  Yes, this is overkill, since
we don't know if 'clsP8' points at a 'MyClass1' or not, but this is the
same overkill as we already have with root classes themselves, as 
mentioned
above.

>
> I did add a flag to lookup_method_in_protocol_list to indicate whether
> instance methods of protocols adopted by root classes should be
> searched, so the caller can decide.  This is necessary as we only want
> to use this feature if we actually have 'Class', not when we have a
> "concrete" MyClass4 that is not a root class but implements a protocol
> that is a root class of a different hierarchy:
>
> @protocol MyProto1
> @end

You mean

   @protocol MyProto1
   -doItInstance1;
   @end

right?

>
> @interface MyClass1 <MyProto1> /* root */
> @end
> @interface MyClass2 : MyClass1
> @end
> @interface MyClass3 /* New root */
> @end
> @interface MyClass4 : MyClass3 <MyProto1>
> @end
>
>   [MyClass2 doItInstance1]; /* Do not warn */
> ...
>   [MyClass4 doItInstance1]; /* { dg-warning "may not respond to" } */
>

In this case, the lookup should be performed by lookup_method_static(), 
which I
believe already does what it should.  I agree with you that the warning 
_is_
needed in the second case, but lookup_method_static() should _not_ be 
able to
stumble upon MyProto1 while searching for +doItInstance1 in MyClass4; 
if it does
somehow find it, then I think it's buggy. :-(  So again, I don't think 
you
need to do anything beyond what's already being done to handle this 
case.

Anyway, thanks for putting up with my critique.  The c-parse.in portion 
of your
patch is right on the money, BTW.

--Zem



More information about the Gcc-patches mailing list