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