This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: Your 'Class <Protocol>' Work


Ziemowit Laski wrote:
> On 29 Sep 2004, at 15.00, d.ayers@inode.at wrote:
> 
> 
> No, I don't think that patch will add the space missing between 
> 'add_method_to_hash_list' and '(' in the line above.  And, you
> obviously have something other than a single space following the ','
> -- it could be a single tab, but that still should be changed.

My bad.  Don't know what I was looking at/for yesterday.

>>> 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?
>>
>>I'd rather keep the search correct wrt diagnostics and really warn if
>>
>>@protocol Proto3
>>-someMessage3;
>>@end
>>@interface Root2 <Proto3>
>>@end
>>
>>Class <Proto1, Proto2> obj;
>>[obj someMessage3];
>>
>>from the type information we have the object won't respond.
> 
> 
> Well, I think that we ought to have behavior here that is analogous to
> what would happen if we use 'id <Proto1, Proto2>' instead.  I tried
> substituting 'id' for 'Class' in your example, and the compiler
> spewed out
> 
>    warning: `-someMessage3' not implemented by protocol(s)
> 
> which is a somewhat narrower indictment than saying that the object
> may not respond to the message at all.  Now, if we put 'Class' back
> in place of 'id', I'd argue the compiler should say
> 
>    warning: `+someMessage3' not implemented by protocol(s)
> 
> since, narrowly speaking, neither Proto1 nor Proto2 provide 
> +someMessage3.
> 
> Of course, I now realize that the example that I gave you _should_,
> in fact, provide a diagnostic:
> 
>    warning: `+someMessage' not implemented by protocol(s)
> 
> even though there is a '-someMessage' that will (probably) wind up 
> being used.
> 
> This, of course, leads to an even greater epiphany :-) -- messaging
> 'id <ProtoList>' or 'Class <ProtoList>' is simpler in that it only
> checks if ProtoList provides the method (instance method in case of
> 'id <ProtoList>', class method in case of 'Class <ProtoList>') and if
> it does not, it generates a warning _even if_ some root class
> somewhere (and/or protocol thereto attached) happens to provide such
> a method.
> 
> I apologize for not realizing this earlier; I kept thinking of when
> to issue the warning 'Class <Proto1, Proto2> may not respond to
> "bar"' and when to suppress it. A brief lecture of objc-act.c now
> reminded me that such a warning should _never_ be issued for
> receivers of type 'id' or 'Class', whether protocol-qualified or
> not.

OK, now we either have a fundamental disagreement or a fundamental
misunderstanding.  If I send a message to a protocol qualified 'id'
which is not declared by the protocol, I expect a warning.  Why else
should I bother qualifying it?  Or what do you mean with "that such a
warning should _never_ be issued for receivers of type 'id' or 'Class',
_whether protocol-qualified or not_"?

For the 'Class' case the situation a bit more hairy, due to the
semantics of instance methods in root classes.  What I'm trying to
achieve is that:

Class <NSObject> clsP1;
Class <NSCoding> clsP2;

[clsP1 retain];                 /* doesn't warn */
[clsP2 encodeWithCoder: coder]; /* warns */

Now if you are saying that you think it's OK, to warn in the +retain
case, (which is what I  understand from the second to last paragraph)
then, yes, we wouldn't need to mark root protocols at all.  But as the
correct retrieval of that prototype and the suppression of that warning
is _exactly_ what I'm trying to achieve, I have written the code to mark
those protocols and to test that mark when searching for protocols.
This is especially important when the prototypes start mismatching which
was part of the original trigger for me to do this at all.

> 
> In summation, I believe that the only necessary changes to 
> objc_finish_message_expr() are the following (I've attached it inline
> to preserve tabulation):
> 
> 
> 
> ------------------------------------------------------------------------
> 
> Index: gcc/objc/objc-act.c
> ===================================================================
> RCS file: /cvs/gcc/gcc/gcc/objc/objc-act.c,v
> retrieving revision 1.248
> diff -u -3 -p -r1.248 objc-act.c
> --- gcc/objc/objc-act.c	24 Sep 2004 23:15:33 -0000	1.248
> +++ gcc/objc/objc-act.c	30 Sep 2004 01:13:57 -0000
> @@ -5570,7 +5570,8 @@ objc_finish_message_expr (tree receiver,
>        else
>  	{
>  	  class_tree = objc_class_name;
> -	  OBJC_SET_TYPE_NAME (rtype, class_tree);
> +	  rprotos = TYPE_PROTOCOL_LIST (rtype);
> +	  rtype = NULL_TREE;
>  	}
>  
>        if (rprotos)
> @@ -5650,10 +5651,19 @@ objc_finish_message_expr (tree receiver,
>  		 IDENTIFIER_POINTER (OBJC_TYPE_NAME (rtype)),
>  		 (class_tree ? '+' : '-'),
>  		 IDENTIFIER_POINTER (sel_name));
> +      /* If we are messaging an unadorned 'id' or 'Class' object,
> +	 then we have failed to find _any_ instance or class method,
> +	 respectively.  */
> +      else if (!rprotos)
> +	warning ("no `%c%s' method found",
> +		 (class_tree ? '+' : '-'),
> +		 IDENTIFIER_POINTER (sel_name));
> +
>        if (rprotos)
>  	warning ("`%c%s' not implemented by protocol(s)",
>  		 (class_tree ? '+' : '-'),
>  		 IDENTIFIER_POINTER (sel_name));
> +		 
>        if (!warn_missing_methods)
>  	{
>  	  warning ("(Messages without a matching method signature");
> 
> 
> ------------------------------------------------------------------------

I tried it anyway and it fails to find the prototypes.  Since you have
the patch and test case maybe you can play a bit, to show me what you mean.

> At this point, I'm wondering whether (1)
> PROTOCOL_ADOPTED_BY_ROOT_CLASS is needed at all and (2) if instance
> methods of protocols adopted by root classes should be placed in the
> class method hash table. Doing so will make no difference whatsoever
> when messaging 'Class <ProtoList>', but may help out when messaging
> an unadorned 'Class'. What do you think? I have yet to develop a
> strong opinion here... :-)

As noted above, it does make a difference.  Or I'm totally missing your
point.

>>>Perhaps instead of 'incorporated', we should consistently say
>>>'inherited'?
>>
>> Actually "incorporated" is the term used in the "Objective-C" book.
>>  
>> http://developer.apple.com/documentation/Cocoa/Conceptual/ObjectiveC/
>>  LanguageSummary/chapter_5_section_8.html And the term "inherited"
>> is a bit misleading. The concept doesn't really match protocols.
> 
> 
> Why not? If you think of protocols as abstract interfaces, then 
> "inherit" fits the bill perfectly, I would think... Personally, I
> find "incorporate" a bit vague, since it could mean that you're
> manually repeating the method declarations from the other
> protocol...

Well I'm not going to argue heavily about this.  I think "repeating the
method declarations" is exactly what it does, but then again
"inheritance" repeats instance variables, method declarations and
implementation.  There is a difference to Objective-C class inheritance
in that you can declare "multiple inheritance" wrt to protocol
declarations.  I just had the feeling that the term when applied to
protocols was causing confusion.  I'm still partial to term
"incorporated" now, but if you'd like me to revert my documentation to
speak of inheritance, I will.

BTW, :-) If you feel strongly about conceptually representing protocols
as a second set of inheritance hierarchies, then maybe we should
reconsider whether:

@protocol MyProto8
+(void)doItClass8;
-(void)doItInstance8;
@end
@protocol MyProto9 <MyProto8>
+(void)doItClass9;
-(void)doItInstance9;
@end

where MyProto9 is a "subprotocol" of MyProto8

@interface SomeRoot <MyProto9>
@end

Class <MyProto8> clsP8 = 0;

[clsP8 doItInstance8];

should warn, or not, as strictly speaking it's only the "sub protocol"
that has been declared by a root class and not MyProto8.  But like I
said the case is extreme and we save ourselves a lot of complexity by
not handling it.  I just mentioned it again to show the potential
expectation if you view protocols as (mulitple inheritance) hierarchies,
as I also did, which lead to my first implementation.

...
[sniped sly remark]
> You're not being constructive here.  :-(

Sorry, it was intended to be a little pinch, not a slap.

> If, as you say, the term "inherited" is misleading, then it
> definitely _is_ interesting to see what term the Java-ites settled on
>  in order to maintain this distinction and avoid confusion. Just as
> Java "inherited" (pun intended) the notion of protocols/interfaces
> from ObjC, so we can definitely borrow their terminology if we find
> it useful.

<joke>I do Objective-C, I don't believe in multiple inheritance.  (not
implying that Java has it) :-) Well in this case it would even be
"recursive".</joke>

No, seriously, if the Java folks have a term for this, let's hear 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?
>>
>> Well yes, I agree that they are virtually identical, which is why
>> it was so easy to "write" objc_comptypes_proto_proto. Maybe I
>> should have mentioned that this is a verbatim copy if the previuos
>> handling in the "Protocol" = "Protocol" case, plus the assertion
>> that both parameters must either be 'id' or both be 'Class'.
> 
> 
> OK, but then objc_comptypes_proto_proto() is nothing more than a 
> gratuitous rewrite of objc_comptypes(), which I thought we agreed
> should wait until later... :-)
> 

Actually, objc_comptypes_proto_proto is merely an extraction of a part
of objc_comptypes to avoid code duplication and avoid rearranging the
flow control of objc_comptypes. There is no change in the real code, it
just cut & paste and reformatted to take the nesting level into account.
 That doesn't fight my definition of a gratuitous rewrite.

> I'm curious... if you were just to augment the definition of 
> IS_PROTOCOL_QUALIFIED_ID to encompass 'Class <Proto>' constructs, but
> left objc_comptypes() in its original form, which comparisons would
> break as a result?

objc_comptypes is currently structured like:

if (lhs is pointer to structure && rhs is pointer to structure)
  {
    if (lhs is protocol qualified 'id')
      {
        if (rhs is protocol qualified 'id')
          { Handle "id <protocol> = id <protocol>" case. }
        else if (rhs is concrete class reference)
          { Handle "id <protocol> = SomeClass *" case. }
        else if (rhs is 'id')
          { Handle "id <protocol> = id" case. }
        else if (rhs is 'Class')
          { Handle "id <protocol> = Class" case. }
        else
          { Defer handling to comptypes. }
      }
    else if (rhs is protocol qualified 'id')
      {
        if (rhs is concrete class reference)
          { Handle "SomeClass * = id <protocol>" case. }
        else if (rhs is 'id')
          { Handle "id = id <protocol>" case. }
        else if (rhs is 'Class')
          { Handle "Class = id <protocol>" case. }
        else
          { Defer handling to comptypes. }
       }
     else
       { strip pointer from lhs and rhs }
  }

...
What I've done is:

if (lhs is pointer to structure && rhs is pointer to structure)
  {
    if (lhs is protocol qualified 'id')
      {
        if (rhs is protocol qualified 'id')
*         { Handle "id <protocol> = id <protocol>" case. }
        else if (rhs is concrete class reference)
          { Handle "id <protocol> = SomeClass *" case. }
        else if (rhs is 'id')
          { Handle "id <protocol> = id" case. }
        else if (rhs is 'Class')
          { Handle "id <protocol> = Class (& Class <protocol>)" case. }
        else
          { Defer handling to comptypes. }
      }
    else if (rhs is protocol qualified 'id')
      {
        if (rhs is concrete class reference)
          { Handle "SomeClass * = id <protocol>" case. }
        else if (rhs is 'id')
          { Handle "id = id <protocol>" case. }
        else if (rhs is 'Class')
          { Handle "Class (& Class <protocol>) = id <protocol> " case. }
        else
          { Defer handling to comptypes. }
      }
+   else if (lhs is protocol qualified 'Class')
+     {
+       if (rhs is protocol qualified 'Class')
+*        { Handle "Class <protocol> = Class <protocol>" case. }
+       else if (rhs is concrete class reference)
+         { Handle "Class <protocol> = SomeClass *" case. }
+       else if (rhs is 'id')
+         { Handle "Class <protocol> = id" case. }
+       else if (rhs is 'Class')
+         { Handle "Class <protocol> = Class" case. }
+       else
+         { Defer handling to comptypes. }
+     }
+   else if (rhs is protocol qualified 'Class')
+     {
+       if (rhs is concrete class reference)
+         { Handle "SomeClass * = Class <protocol>" case. }
+       else if (rhs is 'id')
+         { Handle "id = Class <protocol>" case. }
+       else if (rhs is 'Class')
+         { Handle "Class = Class <protocol>" case. }
+       else
+         { Defer handling to comptypes. }
+     }
     else
       { strip pointer from lhs and rhs }
  }

with the * being the extracted code in objc_comptypes_proto_proto.

What I /think/ you are asking me to do is:

if (lhs is pointer to structure && rhs is pointer to structure)
  {
    if (lhs is protocol qualified 'id' or 'Class')
      {
        if (rhs is protocol qualified 'id' or 'Class')
(1)       { Handle "id/Class <protocol> = id/Class <protocol>" case. }
        else if (rhs is concrete class reference)
(2)       { Handle "id/Class <protocol> = SomeClass *" case. }
        else if (rhs is 'id')
          { Handle "id/Class <protocol> = id" case. }
        else if (rhs is 'Class')
(3)       { Handle "id/Class <protocol> = Class" case. }
        else
          { Defer handling to comptypes. }
      }
    else if (rhs is protocol qualified 'id' or 'Class')
      {
        if (rhs is concrete class reference)
(4)       { Handle "SomeClass * = id/Class <protocol>" case. }
        else if (rhs is 'id')
          { Handle "id = id/Class <protocol>" case. }
        else if (rhs is 'Class')
(5)        { Handle "Class = id/Class <protocol>" case. }
        else
          { Defer handling to comptypes. }
       }
     else
       { strip pointer from lhs and rhs }
  }

(1) Here we would either have to special case:
Class <protocol> = id <protocol> (and the inverse)
to returns 0 (not compatible) without the need to checking protocol
conformance.

(2) Here we would either have to special case:
Class <protocol> = SomeClass *
To return 0 (not compatible) instead of looking for the protocol in the
class.

(3) Here we would either have to special case:
Class = id <protocol>
which returns 0 (not compatible)
Class = Class <protocol>
should return 1.

(4) Inversion of (2)
(5) Inversion of (3)

And it's this special casing that would disrupt the flow, at least IMO.

>>> 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.
>>
>> I don't think it worth loosing that type information. We need to 
>> account for several root classes each possibly adopting different
>> protocols.
> 
> 
> But lookup_method_static() has access to this information as is; see  
> below.
> 
>>>>@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.
>>
>> Well just because a class conforms to MyProto8 does not mean it is
>> a class who's root class conforms to MyProto9 so we are being
>> rather lax but the case can be a bit pathalogical and the
>> complexity not worth the trouble.
> 
> 
> Yes, I humbly reverse my previous opinion; see above.

Umm, I'm lost.  So is your opinion that MyProto "shouldn't" be a "root"
class?  Or are you just suggesting, that with the above assertion, that
marking root protocols is superfluous?  Which I hope I have shown that
it isn't, that it is in effect very important wrt to "prototype hygiene"
to quote you :-).

>>>> 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?
>>
>>Indeed :-) This was from the test case.
>>
>>
>>>>@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.
>>
>> Yet lookup_method_static uses lookup_method_in_protocol_list, so
>> that is exactly the place where I need to tell it to act
>> differently :-)
> 
> 
> I think, though, that when lookup_method_static() reaches the top of the
> hierarchy when searching for class methods, it will search for instance
> methods in the root class _and_ its associated protocols as a last gasp,
> no?

Exactly!  But it is _not_ supposed to look into the protocols adopted by
unrelated root classes, this what that flag suppresses here.

Cheers,
David


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]