This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: Your 'Class <Protocol>' Work
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