On Jul 13, 2009, at 9:14 AM, steve naroff wrote:
Does this add support for Class<x> and friends? If so, please add
testcases.
It provides almost all of the support. I wanted to finish adding
"Class<x>" in a separate patch. When I do, I'll add test cases.
Ok.
Please don't use static members within the class. This is not safe
when there are multiple translation units open at a time and when
threading is happening. Please put state like this in ASTContext.
If I put the state in ASTContext, then
ObjCObjectPointerType::isObjCIdType() and Type::isObjCIdType() will
need a pointer to it (which is what I was trying to avoid).
For now, I'll just make the change and live with a less convenient
API. We could have a low cost union (by playing games with the low
order bits of the ObjCInterfaceType), however I'd prefer not to
fiddle with this for now.
Ok, going with something simple makes sense to me.
bool ASTContext::canAssignObjCInterfaces(const ObjCInterfaceType
*LHS,
const ObjCInterfaceType
*RHS) {
+ // If either interface represents the built-in 'id' or 'Class'
types,
+ // then return true.
+ if (LHS->isObjCIdInterface() || RHS->isObjCIdInterface() ||
+ LHS->isObjCClassInterface() || RHS->isObjCClassInterface())
"isObjCIdInterface || isObjCClassInterface"
Does it make sense to add direct predicates for these?
Sure. How about isObjCBuiltinType() and isObjCBuiltinInterface()?
Works for me.
trunk/lib/AST/ExprConstant.cpp Fri Jul 10 18:34:53 2009
@@ -382,7 +382,8 @@
const Expr* SubExpr = E->getSubExpr();
// Check for pointer->pointer cast
- if (SubExpr->getType()->isPointerType()) {
+ if (SubExpr->getType()->isPointerType() ||
+ SubExpr->getType()->isObjCObjectPointerType()) {
Would it make sense to add a "isAnyPointerType()" method to do both
these checks?
I don't think so. It will save a few keystrokes, however I believe
the above is clearer.
What about block pointers? getAsPointeeType works with blocks, are
there places that really want to be checking for "c pointer, objc
pointer, or block pointer"? If so, it would make sense to introduce a
predicate for it. To address Sebastian's concern, we would just need
to name the predicate right.
+++ cfe/trunk/lib/CodeGen/CGExprScalar.cpp Fri Jul 10 18:34:53 2009
@@ -987,7 +987,7 @@
}
Value *ScalarExprEmitter::EmitAdd(const BinOpInfo &Ops) {
- if (!Ops.Ty->isPointerType()) {
+ if (!Ops.Ty->isPointerType() && !Ops.Ty-
>isObjCObjectPointerType()) {
Another candidate for "isAnyPointerType"? What happens when you
add an objc pointer to an integer?
You can't have an objc pointer to an integer (that's why I called
the class ObjCObjectPointerType)....an ObjCObjectPointerType can
only refer to an interface (built-in or user-defined).
Maybe I don't understand your question.
I mean something like:
NSString *X;
X+4;
This doesn't seem like the right thing, can you check with Daniel
on this?
Daniel suggested this solution to me (so I guess I've already
checked:-)
Ok.
It seems that the objc and normal pointer type can be merged with
your new "getpointeetype" method.
Makes sense. Note: This is an example of a cleanup that I wanted to
defer (for this mega patch).
Of course, no harm delaying the cleanups.
This code looks like it is something that should be factored out
into a helper method. Are there other pieces of code doing the
same lookup algorithm?
Yep. I agree a helper would be nice. Since this doesn't really
relate to the ObjCObjectPointerType related changes, I'll note this
as a separate cleanup (the only reason in showed up in the patch is
I move the code).
Right, none of my comments really were meant to imply that you should
have included them in the original patch, they were meant for follow
up changes.
@@ -3517,6 +3523,29 @@
return Incompatible;
}
+ if (isa<ObjCObjectPointerType>(lhsType)) {
+ if (rhsType->isIntegerType())
+ return IntToPointer;
+
+ if (isa<PointerType>(rhsType)) {
+ QualType lhptee = lhsType->getAsObjCObjectPointerType()-
>getPointeeType();
+ QualType rhptee = rhsType->getAsPointerType()-
>getPointeeType();
+ return CheckPointeeTypesForAssignment(lhptee, rhptee);
Why not use lhsType->getPointeeType() (and also for rhsType)?
Since the routine is already doing the "isa" sniffing to determine
what types we have, I didn't think it would be any clearer (or
efficient).
+ }
+ if (rhsType->isObjCObjectPointerType()) {
+ QualType lhptee = lhsType->getAsObjCObjectPointerType()-
>getPointeeType();
+ QualType rhptee = rhsType->getAsObjCObjectPointerType()-
>getPointeeType();
+ return CheckPointeeTypesForAssignment(lhptee, rhptee);
That would allow merging this case in as well.
Same response as above.
The point of using the predicate is that it allows merging of these
two if blocks.
@@ -3776,12 +3823,23 @@
// Put any potential pointer into PExp
Expr* PExp = lex, *IExp = rex;
- if (IExp->getType()->isPointerType())
+ if (IExp->getType()->isPointerType() ||
+ IExp->getType()->isObjCObjectPointerType())
->isAnyPointerType()
As a said earlier, I'm not convinced adding another predicate would
make the code clearer (though it would definitely make it more terse).
Maybe a better name would sway my opinion. isCOrObjCPointer() is
more descriptive, however one of the uglier names I've ever seen:-)
Sure, a better name than "isAnyPointerType" is definitely appreciated,
but I think it is useful to consider it. There is a bunch of code
that uses this predicate now.
if (IExp->getType()->isIntegerType()) {
- QualType PointeeTy = PTy->getPointeeType();
+ QualType PointeeTy;
+ const PointerType *PTy;
+ const ObjCObjectPointerType *OPT;
+
+ if ((PTy = PExp->getType()->getAsPointerType()))
+ PointeeTy = PTy->getPointeeType();
+ else if ((OPT = PExp->getType()-
>getAsObjCObjectPointerType()))
+ PointeeTy = OPT->getPointeeType();
This should be able to use QualType::getPointeeType() instead of
the if/elseif
This isn't possible without more code reorganization (I already
tried it). The code that follows is interested in both PTy and OPT
(not only the pointee type).
Ok, I completely missed that. This is really subtle, why not set
"PointeeTy" with a call to the getPointeeType() method, and then
change the places that look at PTy/OPT to query the original type
directly? I think the code would be more clear that way.
+++ cfe/trunk/test/CodeGenObjC/encode-test.m Fri Jul 10 18:34:53
2009
@@ -1,7 +1,7 @@
// RUN: clang-cc -triple=i686-apple-darwin9 -fnext-runtime -emit-
llvm -o %t %s &&
// RUN: grep -e "\^{Innermost=CC}" %t | count 1 &&
-// RUN: grep -e "{Derived=#ib32b8b3b8sb16b8b8b2b8ccb6}" %t |
count 1 &&
-// RUN: grep -e "{b...@c}" %t | count 1 &&
+// RUN: grep -e
"{Derived=^{objc_class}ib32b8b3b8sb16b8b8b2b8ccb6}" %t | count 1 &&
+// RUN: grep -e "{b1=^{objc_cla...@c}" %t | count 1 &&
// RUN: grep -e "v...@0:4\[3...@]]8" %t | count 1 &&
// RUN: grep -e "r\^{S=i}" %t | count 1 &&
// RUN: grep -e "\^{Object=#}" %t | count 1
The output of @encode changed? Isn't this an ABI bug??
This following two issues are a result of moving away from the C-
structure dependency (which is a hack). More info...
We no longer treat "struct objc_class *" as synonymous with "Class".
This is a side-effect of removing
"ASTContext::isObjCClassStructType(T)".
@interface B1
{
struct objc_class *isa;
Int1 *sBase;
char c;
}
@end
Since it does effect GCC binary compatibility, we could certainly
add back a hack to make them synonymous.
I think that we should do this for @encode and C++ mangling.
Compatibility is very important at this level because it isn't a
matter of the compiler rejecting or producing a warning, it is a
silent miscompilation.
+++ cfe/trunk/test/CodeGenObjC/overloadable.m Fri Jul 10 18:34:53
2009
@@ -3,8 +3,8 @@
@class C;
-// RUN: grep _Z1fP11objc_object %t | count 1 &&
-void __attribute__((overloadable)) f(C *c) { }
+// RUN: grep _Z1fP2id %t | count 1 &&
+void __attribute__((overloadable)) f(id c) { }
// RUN: grep _Z1fP1C %t | count 1
-void __attribute__((overloadable)) f(id c) { }
+void __attribute__((overloadable)) f(C *c) { }
Likewise, mangling changing sounds like a serious ABI bug.
Same issue as above (but with ASTContext::isObjCIdStructType()). We
now mangle "id" as "id" (not the underlying structure).
Since it does effect GCC binary compatibility, we could certainly
add back a hack to make them synonymous.
Ok, please do.
+++ cfe/trunk/test/SemaObjC/comptypes-5.m Fri Jul 10 18:34:53 2009
@@ -26,8 +26,8 @@
MyOtherClass<MyProtocol> *obj_c_super_p_q = nil;
MyClass<MyProtocol> *obj_c_cat_p_q = nil;
- obj_c_cat_p = obj_id_p; // expected-warning {{incompatible
type assigning 'id<MyProtocol>', expected 'MyClass *'}}
- obj_c_super_p = obj_id_p; // expected-warning {{incompatible
type assigning 'id<MyProtocol>', expected 'MyOtherClass *'}}
+ obj_c_cat_p = obj_id_p;
+ obj_c_super_p = obj_id_p;
Is this supposed to be allowed?
GCC warns...
test/SemaObjC/comptypes-5.m:29: warning: assignment from distinct
Objective-C type
test/SemaObjC/comptypes-5.m:30: warning: assignment from distinct
Objective-C type
I decided to allow it. Rationale: Both MyClass and MyOtherClass
implement MyProtocol. Since the protocols "match", and you can
assign any 'id' to an interface type (without warning), I decided to
allow it. I'm happy to put back the warning if others feel strongly
(Fariborz?).
I'll let Fariborz make this call.
+++ cfe/trunk/test/SemaObjC/message.m Fri Jul 10 18:34:53 2009
@@ -95,6 +95,6 @@
void foo4() {
struct objc_object X[10];
- [X rect];
+ [(id)X rect];
}
This is a bug, we should be performing unary "array -> pointer"
decay here. This was in response to a bugzilla, we need to support
this.
I don't see the issue here. Both GCC and clang warn if no cast is
used.
[steve-naroffs-imac-3:~/llvm/tools/clang] snaroff% cc -c xx.m
xx.m: In function ‘foo4’:
xx.m:29: warning: invalid receiver type ‘objc_object [10]’
[steve-naroffs-imac-3:~/llvm/tools/clang] snaroff% ../../Debug/bin/
clang -c xx.m
xx.m:29:3: warning: receiver type 'struct objc_object *' is not 'id'
or interface pointer, consider casting it to 'id'
[X rect];
^~
xx.m:29:3: warning: method '-rect' not found (return type defaults
to 'id')
[X rect];
^~~~~~~~
2 diagnostics generated.
Ok, then please fix to test by adding an 'expected-warning' instead of
inserting the explicit cast. The intent of the test is to verify that
passing an array as the receiver works.
+++ cfe/trunk/test/SemaObjCXX/overload.mm Fri Jul 10 18:34:53 2009
@@ -1,4 +1,5 @@
// RUN: clang-cc -fsyntax-only -verify %s
+// XFAIL
@interface Foo
@end
What is the problem with this test?
There were several problems (mostly related in incomplete handling
of ObjC types in the C++ infrastructure). I discussed this with Doug
and we decided to add the XFAIL.
Ok, works for me.
Thanks again for working on this Steve! I know it's largely
thankless, but it's a huge cleanup.
I appreciate the encouragement. I think this kind of hygiene is
quite important. Kind of like getting my teeth cleaned:-)
:)
-Chris
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
