<html><head><style>body{font-family:Helvetica,Arial;font-size:13px}</style></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><div id="bloop_customfont" style="font-family:Helvetica,Arial;font-size:13px; color: rgba(0,0,0,1.0); margin: 0px; line-height: auto;">Thank you for feedback.</div><div id="bloop_customfont" style="font-family:Helvetica,Arial;font-size:13px; color: rgba(0,0,0,1.0); margin: 0px; line-height: auto;">Here I put some comments and answers for you comments and questions</div><div id="bloop_customfont" style="font-family:Helvetica,Arial;font-size:13px; color: rgba(0,0,0,1.0); margin: 0px; line-height: auto;"> (stripped some unrelated code, for a better readability):</div><div id="bloop_customfont" style="font-family:Helvetica,Arial;font-size:13px; color: rgba(0,0,0,1.0); margin: 0px; line-height: auto;"><br></div><div id="bloop_customfont" style="font-family:Helvetica,Arial;font-size:13px; color: rgba(0,0,0,1.0); margin: 0px; line-height: auto;"><div id="bloop_customfont" style="margin: 0px;">>> Have you considered NSEdgeInsets? With this API:</div><div id="bloop_customfont" style="margin: 0px;"><br></div><div id="bloop_customfont" style="margin: 0px;">Well, no, at least not yet. Currently SDK has UIEdgeInsets, which works only on iOS, and </div><div id="bloop_customfont" style="margin: 0px;">NSEdgeInsets, which works for both iOS (8.0 and higher) and OS X (10.10 and higher).</div><div id="bloop_customfont" style="margin: 0px;">This way all the structures should be hardcoded (like NSGeometry right now). </div><div id="bloop_customfont" style="margin: 0px;">I was thinking about better support for the all structures, but didn't find a good solution yet.</div><div id="bloop_customfont" style="margin: 0px;"><br></div><div id="bloop_customfont" style="margin: 0px;">>> Better to return None; here instead of falling though with unnecessary extra checks. It is also more verbose as to the intention.</div><div id="bloop_customfont" style="margin: 0px;"><br></div><div id="bloop_customfont" style="margin: 0px;">Got it. Will update.</div><div id="bloop_customfont" style="margin: 0px;"><br></div><div id="bloop_customfont" style="margin: 0px;">+ if (T->isVoidPointerType())</div><div id="bloop_customfont" style="margin: 0px;">+ return NSAPI::NSValueWithPointer;</div><div id="bloop_customfont" style="margin: 0px;">+ if (T->isObjCObjectPointerType())</div><div id="bloop_customfont" style="margin: 0px;"><br></div><div id="bloop_customfont" style="margin: 0px;">>> You are intentionally using this API instead of T->isObjCIdType(), right? If so, please add comment.</div><div id="bloop_customfont" style="margin: 0px;"><br></div><div id="bloop_customfont" style="margin: 0px;">Yes, this check covers both `id` and `ObjCObject *` types. Will add comment.</div><div id="bloop_customfont" style="margin: 0px;"><br></div><div id="bloop_customfont" style="margin: 0px;"> CodeGenFunction::EmitObjCBoxedExpr(const ObjCBoxedExpr *E) {</div><div id="bloop_customfont" style="margin: 0px;"> // Generate the correct selector for this literal's concrete type.</div><div id="bloop_customfont" style="margin: 0px;">- const Expr *SubExpr = E->getSubExpr();</div><div id="bloop_customfont" style="margin: 0px;"> // Get the method.</div><div id="bloop_customfont" style="margin: 0px;"> const ObjCMethodDecl *BoxingMethod = E->getBoxingMethod();</div><div id="bloop_customfont" style="margin: 0px;"> assert(BoxingMethod && "BoxingMethod is null");</div><div id="bloop_customfont" style="margin: 0px;">@@ -73,12 +73,9 @@ CodeGenFunction::EmitObjCBoxedExpr(const ObjCBoxedExpr *E) {</div><div id="bloop_customfont" style="margin: 0px;"> CGObjCRuntime &Runtime = CGM.getObjCRuntime();</div><div id="bloop_customfont" style="margin: 0px;"> const ObjCInterfaceDecl *ClassDecl = BoxingMethod->getClassInterface();</div><div id="bloop_customfont" style="margin: 0px;"> llvm::Value *Receiver = Runtime.GetClass(*this, ClassDecl);</div><div id="bloop_customfont" style="margin: 0px;">- </div><div id="bloop_customfont" style="margin: 0px;">- const ParmVarDecl *argDecl = *BoxingMethod->param_begin();</div><div id="bloop_customfont" style="margin: 0px;">- QualType ArgQT = argDecl->getType().getUnqualifiedType();</div><div id="bloop_customfont" style="margin: 0px;">- RValue RV = EmitAnyExpr(SubExpr);</div><div id="bloop_customfont" style="margin: 0px;">+</div><div id="bloop_customfont" style="margin: 0px;"> CallArgList Args;</div><div id="bloop_customfont" style="margin: 0px;">- Args.add(RV, ArgQT);</div><div id="bloop_customfont" style="margin: 0px;">+ EmitCallArgs(Args, BoxingMethod, E->arg_begin(), E->arg_end());</div><div id="bloop_customfont" style="margin: 0px;"><br></div><div id="bloop_customfont" style="margin: 0px;">>> Isn’t this something you can check-in before the overall patch can be checked in?</div><div id="bloop_customfont" style="margin: 0px;"><br></div><div id="bloop_customfont" style="margin: 0px;">Yes, sent another patch (but forgot to specify [PATCH] in the message title):</div><div id="bloop_customfont" style="margin: 0px;">http://thread.gmane.org/gmane.comp.compilers.clang.scm/113033</div><div id="bloop_customfont" style="margin: 0px;"> </div><div id="bloop_customfont" style="margin: 0px;"> .Case("objc_boxed_expressions", LangOpts.ObjC2)</div><div id="bloop_customfont" style="margin: 0px;">+ .Case("objc_boxed_nsvalue_expressions", LangOpts.ObjC2)</div><div id="bloop_customfont" style="margin: 0px;"><br></div><div id="bloop_customfont" style="margin: 0px;">>> Presumable the new __has_feature need be documented somewhere.</div><div id="bloop_customfont" style="margin: 0px;"><br></div><div id="bloop_customfont" style="margin: 0px;">Will update documentation.</div><div id="bloop_customfont" style="margin: 0px;"><br></div><div id="bloop_customfont" style="margin: 0px;">>> Also, why can’t place this under the umbrella objc_boxed_expressions?</div><div id="bloop_customfont" style="margin: 0px;"><br></div><div id="bloop_customfont" style="margin: 0px;">Version 3.5, for example, supports objc_boxed_expression but not NSValue+boxed_expressions, </div><div id="bloop_customfont" style="margin: 0px;">which might cause weird compilation fails. Or did I get it wrong?</div><div id="bloop_customfont" style="margin: 0px;"><br></div><div id="bloop_customfont" style="margin: 0px;">+ // Otherwise, require a declaration of NSValue.</div><div id="bloop_customfont" style="margin: 0px;">+ S.Diag(Loc, diag::err_undeclared_nsvalue);</div><div id="bloop_customfont" style="margin: 0px;">+ return nullptr;</div><div id="bloop_customfont" style="margin: 0px;">+ }</div><div id="bloop_customfont" style="margin: 0px;">+ } else if (!S.NSValueDecl->hasDefinition()) {</div><div id="bloop_customfont" style="margin: 0px;">+ S.Diag(Loc, diag::err_undeclared_nsvalue);</div><div id="bloop_customfont" style="margin: 0px;"><br></div><div id="bloop_customfont" style="margin: 0px;">>> Maybe we should have a clearer diagnostic here.</div><div id="bloop_customfont" style="margin: 0px;"><br></div><div id="bloop_customfont" style="margin: 0px;">Makes sense, I used NSNumber' implementation here. I'd appreciate any suggestions or advice on </div><div id="bloop_customfont" style="margin: 0px;">how to improve diagnostic here (and, probably, for NSNumber)</div><div id="bloop_customfont" style="margin: 0px;"><br></div><div id="bloop_customfont" style="margin: 0px;"> /// BuildObjCNumericLiteral - builds an ObjCBoxedExpr AST node for the</div><div id="bloop_customfont" style="margin: 0px;"> /// numeric literal expression. Type of the expression will be "NSNumber *”.</div><div id="bloop_customfont" style="margin: 0px;"><br></div><div id="bloop_customfont" style="margin: 0px;">>> Comment need be updated.</div><div id="bloop_customfont" style="margin: 0px;"><br></div><div id="bloop_customfont" style="margin: 0px;">Got it. Will update.</div><div id="bloop_customfont" style="margin: 0px;"><br></div><div id="bloop_customfont" style="margin: 0px;"> ExprResult Sema::BuildObjCNumericLiteral(SourceLocation AtLoc, Expr *Number) {</div><div id="bloop_customfont" style="margin: 0px;">@@ -456,10 +536,55 @@ ExprResult Sema::BuildObjCBoxedExpr(SourceRange SR, Expr *ValueExpr) {</div><div id="bloop_customfont" style="margin: 0px;"> }</div><div id="bloop_customfont" style="margin: 0px;"> ValueExpr = RValue.get();</div><div id="bloop_customfont" style="margin: 0px;"> QualType ValueType(ValueExpr->getType());</div><div id="bloop_customfont" style="margin: 0px;">- if (const PointerType *PT = ValueType->getAs<PointerType>()) {</div><div id="bloop_customfont" style="margin: 0px;">- QualType PointeeType = PT->getPointeeType();</div><div id="bloop_customfont" style="margin: 0px;">- if (Context.hasSameUnqualifiedType(PointeeType, Context.CharTy)) {</div><div id="bloop_customfont" style="margin: 0px;">+ if (ValueType->isBuiltinType() || ValueType->isEnumeralType()) {</div><div id="bloop_customfont" style="margin: 0px;"><br></div><div id="bloop_customfont" style="margin: 0px;">>> You are adding an extra check for isEnumeralType() beyond the original source. Please add</div><div id="bloop_customfont" style="margin: 0px;">>> comment for it and add test to back up the comment.</div><div id="bloop_customfont" style="margin: 0px;"><br></div><div id="bloop_customfont" style="margin: 0px;">Previously there was another if branch for Enums, so code looked like</div><div id="bloop_customfont" style="margin: 0px;"><br></div><div id="bloop_customfont" style="margin: 0px;">if char pointer</div><div id="bloop_customfont" style="margin: 0px;"> handle NSString</div><div id="bloop_customfont" style="margin: 0px;">else if builtin type</div><div id="bloop_customfont" style="margin: 0px;"> handle NSNumber</div><div id="bloop_customfont" style="margin: 0px;">else if enum</div><div id="bloop_customfont" style="margin: 0px;"> handle NSNumber</div><div id="bloop_customfont" style="margin: 0px;"><br></div><div id="bloop_customfont" style="margin: 0px;">I just merged the two NSNumber related branches.</div><div id="bloop_customfont" style="margin: 0px;">But, yes, I will update comment and will try to add test.</div><div id="bloop_customfont" style="margin: 0px;"><br></div><div id="bloop_customfont" style="margin: 0px;">- </div><div id="bloop_customfont" style="margin: 0px;">+</div><div id="bloop_customfont" style="margin: 0px;"><br></div><div id="bloop_customfont" style="margin: 0px;">>> Please remove these diffs</div><div id="bloop_customfont" style="margin: 0px;"><br></div><div id="bloop_customfont" style="margin: 0px;">Got it. It was caused by `git diff --check ...`</div><div id="bloop_customfont" style="margin: 0px;"><br></div><div id="bloop_customfont" style="margin: 0px;">+// OS X Specific</div><div id="bloop_customfont" style="margin: 0px;">++ (NSValue *)valueWithPoint:(NSPoint)point;</div><div id="bloop_customfont" style="margin: 0px;">++ (NSValue *)valueWithSize:(NSSize)size;</div><div id="bloop_customfont" style="margin: 0px;">++ (NSValue *)valueWithRect:(NSRect)rect;</div><div id="bloop_customfont" style="margin: 0px;"><br></div><div id="bloop_customfont" style="margin: 0px;">>> It is better to add macosx availability attribute here.</div><div id="bloop_customfont" style="margin: 0px;"><br></div><div id="bloop_customfont" style="margin: 0px;">Makes sense, will update.</div><div id="bloop_customfont" style="margin: 0px;"><br></div><div id="bloop_customfont" style="margin: 0px;">+// iOS Specific</div><div id="bloop_customfont" style="margin: 0px;">++ (NSValue *)valueWithCGPoint:(CGPoint)point;</div><div id="bloop_customfont" style="margin: 0px;">++ (NSValue *)valueWithCGSize:(CGSize)size;</div><div id="bloop_customfont" style="margin: 0px;">++ (NSValue *)valueWithCGRect:(CGRect)rect;</div><div id="bloop_customfont" style="margin: 0px;">+</div><div id="bloop_customfont" style="margin: 0px;"><br></div><div id="bloop_customfont" style="margin: 0px;">>> It is better to add iOS availability attribute here.</div><div id="bloop_customfont" style="margin: 0px;"><br></div><div id="bloop_customfont" style="margin: 0px;">Got it.</div><div id="bloop_customfont" style="margin: 0px;"><br></div><div id="bloop_customfont" style="margin: 0px;">+// RUN: %clang_cc1 -I %S/Inputs -triple x86_64-apple-darwin10 -emit-llvm -fblocks -fobjc-arc -fobjc-runtime-has-weak -O2 -disable-llvm-optzns -o - %s | FileCheck %s</div><div id="bloop_customfont" style="margin: 0px;"><br></div><div id="bloop_customfont" style="margin: 0px;">>> If you are not using ‘weak’ objects no need to provide -fobjc-runtime-has-weak </div><div id="bloop_customfont" style="margin: 0px;"><br></div><div id="bloop_customfont" style="margin: 0px;">Copy-paste >_<'</div><div id="bloop_customfont" style="margin: 0px;">Will update.</div><div id="bloop_customfont" style="margin: 0px;"><br></div><div id="bloop_customfont" style="margin: 0px;">>> Need one for iOS with its own checks.</div><div id="bloop_customfont" style="margin: 0px;"><br></div><div id="bloop_customfont" style="margin: 0px;">Got it. Will update.</div><div id="bloop_customfont" style="margin: 0px;"><br></div><div id="bloop_customfont" style="margin: 0px;">+// OS X Specific</div><div id="bloop_customfont" style="margin: 0px;">+// CHECK: [[METH:@.*]] = private global{{.*}}valueWithPoint:{{.*}}</div><div id="bloop_customfont" style="margin: 0px;">+// CHECK-NEXT: [[POINT_SEL:@.*]] = {{.*}}[[METH]]{{.*}}</div><div id="bloop_customfont" style="margin: 0px;">+// CHECK: [[METH:@.*]] = private global{{.*}}valueWithSize:{{.*}}</div><div id="bloop_customfont" style="margin: 0px;">+// CHECK-NEXT: [[SIZE_SEL:@.*]] = {{.*}}[[METH]]{{.*}}</div><div id="bloop_customfont" style="margin: 0px;">+// CHECK: [[METH:@.*]] = private global{{.*}}valueWithRect:{{.*}}</div><div id="bloop_customfont" style="margin: 0px;">+// CHECK-NEXT: [[RECT_SEL:@.*]] = {{.*}}[[METH]]{{.*}}</div><div id="bloop_customfont" style="margin: 0px;"><br></div><div id="bloop_customfont" style="margin: 0px;">>> No. Please have two distinct runs and checks for iOS and macosx. </div><div id="bloop_customfont" style="margin: 0px;"><br></div><div id="bloop_customfont" style="margin: 0px;">Got it. Will update.</div><div id="bloop_customfont" style="margin: 0px;"><br></div><div id="bloop_customfont" style="margin: 0px;">+</div><div id="bloop_customfont" style="margin: 0px;">+// iOS Specfific</div><div id="bloop_customfont" style="margin: 0px;">+// CHECK: [[METH:@.*]] = private global{{.*}}valueWithCGPoint:{{.*}}</div><div id="bloop_customfont" style="margin: 0px;">+// CHECK-NEXT: [[CGPOINT_SEL:@.*]] = {{.*}}[[METH]]{{.*}}</div><div id="bloop_customfont" style="margin: 0px;">+// CHECK: [[METH:@.*]] = private global{{.*}}valueWithCGSize:{{.*}}</div><div id="bloop_customfont" style="margin: 0px;">+// CHECK-NEXT: [[CGSIZE_SEL:@.*]] = {{.*}}[[METH]]{{.*}}</div><div id="bloop_customfont" style="margin: 0px;">+// CHECK: [[METH:@.*]] = private global{{.*}}valueWithCGRect:{{.*}}</div><div id="bloop_customfont" style="margin: 0px;">+// CHECK-NEXT: [[CGRECT_SEL:@.*]] = {{.*}}[[METH]]{{.*}}</div><div id="bloop_customfont" style="margin: 0px;">+</div><div id="bloop_customfont" style="margin: 0px;"><br></div><div id="bloop_customfont" style="margin: 0px;">>> No. Please have two distinct runs and checks for iOS and macosx. </div><div id="bloop_customfont" style="margin: 0px;"><br></div><div id="bloop_customfont" style="margin: 0px;">Got it. Will update.</div><div id="bloop_customfont" style="margin: 0px;"><br></div><div id="bloop_customfont" style="margin: 0px;">>> Sema tests should check for every erroneous situations. Like when NSValue is not available or does not have definition.</div><div id="bloop_customfont" style="margin: 0px;">>> Or when APIs is not available for iOS or Macosx. </div><div id="bloop_customfont" style="margin: 0px;">>> Also need a test for __has_feature.</div><div id="bloop_customfont" style="margin: 0px;"><br></div><div id="bloop_customfont" style="margin: 0px;">Got it. Will add more tests.</div><div id="bloop_customfont" style="margin: 0px;"><br></div></div> <div id="bloop_sign_1418113091219665152" class="bloop_sign"><div style="font-family:helvetica,arial;font-size:13px">-- <br>AlexDenisov</div><div style="font-family:helvetica,arial;font-size:13px">Software Engineer, <a href="https://github.com/AlexDenisov">https://github.com/AlexDenisov</a></div></div> <br><p style="color:#000;">On 8 Dec 2014 at 21:11:55, jahanian (<a href="mailto:fjahanian@apple.com">fjahanian@apple.com</a>) wrote:</p> <blockquote type="cite" class="clean_bq"><span><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><div></div><div>
<title></title>
<div class="">Overall looks very nice. Some comments below. (I have
tried to make them easier to find by prepending them with
>>).</div>
<div class=""><br></div>
</div></div></span></blockquote></body></html>