On Jul 13, 2009, at 4:18 PM, Chris Lattner wrote:
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.
I'm sure there are places where block pointers should be included.
I agree that a predicate would be helpful (if we can come up with a
clear name/semantic).
+++ 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;
I see...I guess I read your response too quickly (sorry for the
confusion).
clang allows this (for GCC compatibility). I'd have no problem
rejecting code like this (since it is silly):
@interface XX
-self;
@end
static void func() {
XX *x;
[x+4 self];
}
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.
Agreed...will consider.
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.
I agree. Will do.
+++ 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.
O.K...will reintroduce a hack to treat "struct objc_object" and
"struct objc_class" specially. Oh well:-)
+++ 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.
Will do.
+++ 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
