<html><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div><div>On Jul 13, 2009, at 9:14 AM, steve naroff wrote:</div><blockquote type="cite"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div><blockquote type="cite"><div>Does this add support for Class<x> and friends? If so, please add testcases.<br><br></div></blockquote><div><br></div>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.</div></div></blockquote><div><br></div>Ok.</div><div><br></div><div><blockquote type="cite"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div><blockquote type="cite"><div>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.<br><br></div></blockquote><div><br></div>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).</div><div><br></div><div>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.</div></div></blockquote><div><br></div>Ok, going with something simple makes sense to me.</div><div><br></div><div><blockquote type="cite"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div><blockquote type="cite"><div><blockquote type="cite">bool ASTContext::canAssignObjCInterfaces(const ObjCInterfaceType *LHS,<br></blockquote><blockquote type="cite"> const ObjCInterfaceType *RHS) {<br></blockquote><blockquote type="cite">+ // If either interface represents the built-in 'id' or 'Class' types,<br></blockquote><blockquote type="cite">+ // then return true.<br></blockquote><blockquote type="cite">+ if (LHS->isObjCIdInterface() || RHS->isObjCIdInterface() ||<br></blockquote><blockquote type="cite">+ LHS->isObjCClassInterface() || RHS->isObjCClassInterface())<br></blockquote><br><br>"isObjCIdInterface || isObjCClassInterface"<br><br>Does it make sense to add direct predicates for these?</div></blockquote><div><br></div>Sure. How about isObjCBuiltinType() and isObjCBuiltinInterface()?</div></div></blockquote><div><br></div>Works for me.</div><div><br><blockquote type="cite"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div><blockquote type="cite"><div><blockquote type="cite">trunk/lib/AST/ExprConstant.cpp Fri Jul 10 18:34:53 2009<br></blockquote><blockquote type="cite">@@ -382,7 +382,8 @@<br></blockquote><blockquote type="cite"> const Expr* SubExpr = E->getSubExpr();<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite"> // Check for pointer->pointer cast<br></blockquote><blockquote type="cite">- if (SubExpr->getType()->isPointerType()) {<br></blockquote><blockquote type="cite">+ if (SubExpr->getType()->isPointerType() ||<br></blockquote><blockquote type="cite">+ SubExpr->getType()->isObjCObjectPointerType()) {<br></blockquote><br>Would it make sense to add a "isAnyPointerType()" method to do both these checks?</div></blockquote><div><br></div>I don't think so. It will save a few keystrokes, however I believe the above is clearer.</div></div></blockquote><div><br></div><div>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.</div><div><br></div><blockquote type="cite"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div><blockquote type="cite"><div><blockquote type="cite"><font class="Apple-style-span" color="#000000"><br></font></blockquote><blockquote type="cite">+++ cfe/trunk/lib/CodeGen/CGExprScalar.cpp Fri Jul 10 18:34:53 2009<br></blockquote><blockquote type="cite">@@ -987,7 +987,7 @@<br></blockquote><blockquote type="cite">}<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">Value *ScalarExprEmitter::EmitAdd(const BinOpInfo &Ops) {<br></blockquote><blockquote type="cite">- if (!Ops.Ty->isPointerType()) {<br></blockquote><blockquote type="cite">+ if (!Ops.Ty->isPointerType() && !Ops.Ty->isObjCObjectPointerType()) {<br></blockquote><br>Another candidate for "isAnyPointerType"? What happens when you add an objc pointer to an integer? </div></blockquote><div><br></div>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).</div><div><br></div><div>Maybe I don't understand your question.</div></div></blockquote><div><br></div><div>I mean something like:</div><div><br></div><div>NSString *X;</div><div> X+4;</div><div><br></div><blockquote type="cite"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div><blockquote type="cite"><div>This doesn't seem like the right thing, can you check with Daniel on this?</div></blockquote><div><br></div>Daniel suggested this solution to me (so I guess I've already checked:-)</div></div></blockquote><div><br></div>Ok.</div><div><br></div><div><blockquote type="cite"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div><blockquote type="cite"><div>It seems that the objc and normal pointer type can be merged with your new "getpointeetype" method.<br><br></div></blockquote><div><br></div>Makes sense. Note: This is an example of a cleanup that I wanted to defer (for this mega patch).</div></div></blockquote><div><br></div>Of course, no harm delaying the cleanups.</div><div><br></div><div><blockquote type="cite"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div><blockquote type="cite"><div>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?<br><br></div></blockquote><div><br></div>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).</div></div></blockquote><div><br></div>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.</div><div><br></div><div><blockquote type="cite"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div><blockquote type="cite"><div><blockquote type="cite">@@ -3517,6 +3523,29 @@<br></blockquote><blockquote type="cite"> return Incompatible;<br></blockquote><blockquote type="cite"> }<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">+ if (isa<ObjCObjectPointerType>(lhsType)) {<br></blockquote><blockquote type="cite">+ if (rhsType->isIntegerType())<br></blockquote><blockquote type="cite">+ return IntToPointer;<br></blockquote><blockquote type="cite">+<br></blockquote><blockquote type="cite">+ if (isa<PointerType>(rhsType)) {<br></blockquote><blockquote type="cite">+ QualType lhptee = lhsType->getAsObjCObjectPointerType()->getPointeeType();<br></blockquote><blockquote type="cite">+ QualType rhptee = rhsType->getAsPointerType()->getPointeeType();<br></blockquote><blockquote type="cite">+ return CheckPointeeTypesForAssignment(lhptee, rhptee);<br></blockquote><br>Why not use lhsType->getPointeeType() (and also for rhsType)?</div></blockquote><div><br></div>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).</div><div><br><blockquote type="cite"><div><br><br><blockquote type="cite">+ }<br></blockquote><blockquote type="cite">+ if (rhsType->isObjCObjectPointerType()) {<br></blockquote><blockquote type="cite">+ QualType lhptee = lhsType->getAsObjCObjectPointerType()->getPointeeType();<br></blockquote><blockquote type="cite">+ QualType rhptee = rhsType->getAsObjCObjectPointerType()->getPointeeType();<br></blockquote><blockquote type="cite">+ return CheckPointeeTypesForAssignment(lhptee, rhptee);<br></blockquote><br>That would allow merging this case in as well.</div></blockquote><div><br></div>Same response as above.</div></div></blockquote><div><br></div><div>The point of using the predicate is that it allows merging of these two if blocks.</div><div><br></div><blockquote type="cite"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div><blockquote type="cite"><div><blockquote type="cite">@@ -3776,12 +3823,23 @@<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite"> // Put any potential pointer into PExp<br></blockquote><blockquote type="cite"> Expr* PExp = lex, *IExp = rex;<br></blockquote><blockquote type="cite">- if (IExp->getType()->isPointerType())<br></blockquote><blockquote type="cite">+ if (IExp->getType()->isPointerType() ||<br></blockquote><blockquote type="cite">+ IExp->getType()->isObjCObjectPointerType())<br></blockquote><br>->isAnyPointerType()</div></blockquote><div><br></div>As a said earlier, I'm not convinced adding another predicate would make the code clearer (though it would definitely make it more terse).</div><div><br></div><div>Maybe a better name would sway my opinion. isCOrObjCPointer() is more descriptive, however one of the uglier names I've ever seen:-)</div></div></blockquote><div><br></div>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.</div><div><br></div><div><blockquote type="cite"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div><blockquote type="cite"><div><blockquote type="cite"> if (IExp->getType()->isIntegerType()) {<br></blockquote><blockquote type="cite">- QualType PointeeTy = PTy->getPointeeType();<br></blockquote><blockquote type="cite">+ QualType PointeeTy;<br></blockquote><blockquote type="cite">+ const PointerType *PTy;<br></blockquote><blockquote type="cite">+ const ObjCObjectPointerType *OPT;<br></blockquote><blockquote type="cite">+<br></blockquote><blockquote type="cite">+ if ((PTy = PExp->getType()->getAsPointerType()))<br></blockquote><blockquote type="cite">+ PointeeTy = PTy->getPointeeType();<br></blockquote><blockquote type="cite">+ else if ((OPT = PExp->getType()->getAsObjCObjectPointerType()))<br></blockquote><blockquote type="cite">+ PointeeTy = OPT->getPointeeType();<br></blockquote><br>This should be able to use QualType::getPointeeType() instead of the if/elseif</div></blockquote><div><br></div>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). </div></div></blockquote><div><br></div><div>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.</div><div><br></div><blockquote type="cite"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div><blockquote type="cite"><div><blockquote type="cite">+++ cfe/trunk/test/CodeGenObjC/encode-test.m Fri Jul 10 18:34:53 2009<br></blockquote><blockquote type="cite">@@ -1,7 +1,7 @@<br></blockquote><blockquote type="cite">// RUN: clang-cc -triple=i686-apple-darwin9 -fnext-runtime -emit-llvm -o %t %s &&<br></blockquote><blockquote type="cite">// RUN: grep -e "\^{Innermost=CC}" %t | count 1 &&<br></blockquote><blockquote type="cite">-// RUN: grep -e "{Derived=#ib32b8b3b8sb16b8b8b2b8ccb6}" %t | count 1 &&<br></blockquote><blockquote type="cite">-// RUN: grep -e "{B1=#@c}" %t | count 1 &&<br></blockquote><blockquote type="cite">+// RUN: grep -e "{Derived=^{objc_class}ib32b8b3b8sb16b8b8b2b8ccb6}" %t | count 1 &&<br></blockquote><blockquote type="cite">+// RUN: grep -e "{B1=^{objc_class}@c}" %t | count 1 &&<br></blockquote><blockquote type="cite">// RUN: grep -e "v12@0:4\[3\[4@]]8" %t | count 1 &&<br></blockquote><blockquote type="cite">// RUN: grep -e "r\^{S=i}" %t | count 1 &&<br></blockquote><blockquote type="cite">// RUN: grep -e "\^{Object=#}" %t | count 1<br></blockquote><br>The output of @encode changed? Isn't this an ABI bug??</div></blockquote><div><br></div><div>This following two issues are a result of moving away from the C-structure dependency (which is a hack). More info...</div><div><br></div><div>We no longer treat "struct objc_class *" as synonymous with "Class". This is a side-effect of removing "ASTContext::isObjCClassStructType(T)".</div><div><br></div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal Monaco; color: rgb(170, 13, 145); ">@interface<span style="color: #000000"> B1 </span></div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal Monaco; ">{</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal Monaco; "> <span style="color: #aa0d91">struct</span> objc_class *isa;</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal Monaco; "> Int1 *sBase;</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal Monaco; "> <span style="color: #aa0d91">char</span> <span style="color: #3f6e74">c</span>;</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal Monaco; ">}</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 10px/normal Monaco; color: rgb(170, 13, 145); ">@end</div><div><font class="Apple-style-span" color="#AA0D91" face="Monaco" size="2"><span class="Apple-style-span" style="font-size: 10px;"><br></span></font></div><div><font class="Apple-style-span" color="#AA0D91" face="Monaco" size="2"><span class="Apple-style-span" style="font-size: 10px;"><span class="Apple-style-span" style="color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; "><div>Since it does effect GCC binary compatibility, we could certainly add back a hack to make them synonymous.</div></span></span></font></div></div></div></blockquote><div><br></div><div>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.</div><br><blockquote type="cite"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div><blockquote type="cite"><div><font class="Apple-style-span" color="#000000"><br></font><blockquote type="cite">+++ cfe/trunk/test/CodeGenObjC/overloadable.m Fri Jul 10 18:34:53 2009<br></blockquote><blockquote type="cite">@@ -3,8 +3,8 @@<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">@class C;<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">-// RUN: grep _Z1fP11objc_object %t | count 1 &&<br></blockquote><blockquote type="cite">-void __attribute__((overloadable)) f(C *c) { }<br></blockquote><blockquote type="cite">+// RUN: grep _Z1fP2id %t | count 1 &&<br></blockquote><blockquote type="cite">+void __attribute__((overloadable)) f(id c) { }<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">// RUN: grep _Z1fP1C %t | count 1<br></blockquote><blockquote type="cite">-void __attribute__((overloadable)) f(id c) { }<br></blockquote><blockquote type="cite">+void __attribute__((overloadable)) f(C *c) { }<br></blockquote><br>Likewise, mangling changing sounds like a serious ABI bug.</div></blockquote><div><br></div>Same issue as above (but with ASTContext::isObjCIdStructType()). We now mangle "id" as "id" (not the underlying structure).</div><div><br></div><div><div>Since it does effect GCC binary compatibility, we could certainly add back a hack to make them synonymous.</div></div></div></blockquote><div><br></div>Ok, please do.</div><div><br></div><div><blockquote type="cite"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div><blockquote type="cite"><div><blockquote type="cite">+++ cfe/trunk/test/SemaObjC/comptypes-5.m Fri Jul 10 18:34:53 2009<br></blockquote><blockquote type="cite">@@ -26,8 +26,8 @@<br></blockquote><blockquote type="cite"> MyOtherClass<MyProtocol> *obj_c_super_p_q = nil;<br></blockquote><blockquote type="cite"> MyClass<MyProtocol> *obj_c_cat_p_q = nil;<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">- obj_c_cat_p = obj_id_p; // expected-warning {{incompatible type assigning 'id<MyProtocol>', expected 'MyClass *'}}<br></blockquote><blockquote type="cite">- obj_c_super_p = obj_id_p; // expected-warning {{incompatible type assigning 'id<MyProtocol>', expected 'MyOtherClass *'}}<br></blockquote><blockquote type="cite">+ obj_c_cat_p = obj_id_p;<br></blockquote><blockquote type="cite">+ obj_c_super_p = obj_id_p;<br></blockquote><br>Is this supposed to be allowed?</div></blockquote><div><br></div>GCC warns...</div><div><br></div><div><div>test/SemaObjC/comptypes-5.m:29: warning: assignment from distinct Objective-C type</div><div>test/SemaObjC/comptypes-5.m:30: warning: assignment from distinct Objective-C type</div><div><br></div><div>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?).</div></div></div></blockquote><div><br></div>I'll let Fariborz make this call.</div><div><br></div><div><blockquote type="cite"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div><blockquote type="cite"><div><blockquote type="cite"><font class="Apple-style-span" color="#000000"><br></font></blockquote><blockquote type="cite">+++ cfe/trunk/test/SemaObjC/message.m Fri Jul 10 18:34:53 2009<br></blockquote><blockquote type="cite">@@ -95,6 +95,6 @@<br></blockquote><blockquote type="cite">void foo4() {<br></blockquote><blockquote type="cite"> struct objc_object X[10];<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">- [X rect];<br></blockquote><blockquote type="cite">+ [(id)X rect];<br></blockquote><blockquote type="cite">}<br></blockquote><br>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.</div></blockquote><div><br></div><div>I don't see the issue here. Both GCC and clang warn if no cast is used.</div><div><br></div><div>[steve-naroffs-imac-3:~/llvm/tools/clang] snaroff% cc -c xx.m</div><div>xx.m: In function ‘foo4’:</div><div>xx.m:29: warning: invalid receiver type ‘objc_object [10]’</div><div><br></div><div>[steve-naroffs-imac-3:~/llvm/tools/clang] snaroff% ../../Debug/bin/clang -c xx.m</div><div>xx.m:29:3: warning: receiver type 'struct objc_object *' is not 'id' or interface pointer, consider casting it to 'id'</div><div> [X rect];</div><div> ^~</div><div>xx.m:29:3: warning: method '-rect' not found (return type defaults to 'id')</div><div> [X rect];</div><div> ^~~~~~~~</div><div>2 diagnostics generated.</div></div></div></blockquote><div><br></div><div>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.</div><br><blockquote type="cite"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div><div><br></div><blockquote type="cite"><div><br><br><blockquote type="cite">+++ cfe/trunk/test/SemaObjCXX/overload.mm Fri Jul 10 18:34:53 2009<br></blockquote><blockquote type="cite">@@ -1,4 +1,5 @@<br></blockquote><blockquote type="cite">// RUN: clang-cc -fsyntax-only -verify %s<br></blockquote><blockquote type="cite">+// XFAIL<br></blockquote><blockquote type="cite">@interface Foo<br></blockquote><blockquote type="cite">@end<br></blockquote><br>What is the problem with this test?</div></blockquote><div><br></div>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.</div></div></blockquote><div><br></div>Ok, works for me.</div><div><br><blockquote type="cite"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div><blockquote type="cite"><div>Thanks again for working on this Steve! I know it's largely thankless, but it's a huge cleanup.</div></blockquote><div><br></div>I appreciate the encouragement. I think this kind of hygiene is quite important. Kind of like getting my teeth cleaned:-)</div></div></blockquote></div><br><div>:)</div><div><br></div><div>-Chris</div></body></html>