[Patch][ObjC][Proposal] NSValue literals

[AlexDenisov]

Thank you for feedback.
Here I put some comments and answers for you comments and questions
 (stripped some unrelated code, for a better readability):

>> Have you considered NSEdgeInsets? With this API:

Well, no, at least not yet. Currently SDK has UIEdgeInsets, which works only on iOS, and 
NSEdgeInsets, which works for both iOS (8.0 and higher) and OS X (10.10 and higher).
This way all the structures should be hardcoded (like NSGeometry right now). 
I was thinking about better support for the all structures, but didn't find a good solution yet.

>> Better to return None; here instead of falling though with unnecessary extra checks. It is also more verbose as to the intention.

Got it. Will update.

+  if (T->isVoidPointerType())
+    return NSAPI::NSValueWithPointer;
+  if (T->isObjCObjectPointerType())

>> You are intentionally using this API instead of T->isObjCIdType(), right? If so, please add comment.

Yes, this check covers both `id` and `ObjCObject *` types. Will add comment.

 CodeGenFunction::EmitObjCBoxedExpr(const ObjCBoxedExpr *E) {
   // Generate the correct selector for this literal's concrete type.
-  const Expr *SubExpr = E->getSubExpr();
   // Get the method.
   const ObjCMethodDecl *BoxingMethod = E->getBoxingMethod();
   assert(BoxingMethod && "BoxingMethod is null");
@@ -73,12 +73,9 @@ CodeGenFunction::EmitObjCBoxedExpr(const ObjCBoxedExpr *E) {
   CGObjCRuntime &Runtime = CGM.getObjCRuntime();
   const ObjCInterfaceDecl *ClassDecl = BoxingMethod->getClassInterface();
   llvm::Value *Receiver = Runtime.GetClass(*this, ClassDecl);
-  
-  const ParmVarDecl *argDecl = *BoxingMethod->param_begin();
-  QualType ArgQT = argDecl->getType().getUnqualifiedType();
-  RValue RV = EmitAnyExpr(SubExpr);
+
   CallArgList Args;
-  Args.add(RV, ArgQT);
+  EmitCallArgs(Args, BoxingMethod, E->arg_begin(), E->arg_end());

>> Isn’t this something you can check-in before the overall patch can be checked in?

Yes, sent another patch (but forgot to specify [PATCH] in the message title):
http://thread.gmane.org/gmane.comp.compilers.clang.scm/113033
 
       .Case("objc_boxed_expressions", LangOpts.ObjC2)
+      .Case("objc_boxed_nsvalue_expressions", LangOpts.ObjC2)

>> Presumable the new __has_feature need be documented somewhere.

Will update documentation.

>> Also, why can’t place this under the umbrella objc_boxed_expressions?

Version 3.5, for example, supports objc_boxed_expression but not NSValue+boxed_expressions, 
which might cause weird compilation fails. Or did I get it wrong?

+        // Otherwise, require a declaration of NSValue.
+        S.Diag(Loc, diag::err_undeclared_nsvalue);
+        return nullptr;
+      }
+    } else if (!S.NSValueDecl->hasDefinition()) {
+      S.Diag(Loc, diag::err_undeclared_nsvalue);

>> Maybe we should have a clearer diagnostic here.

Makes sense, I used NSNumber' implementation here. I'd appreciate any suggestions or advice on 
how to improve diagnostic here (and, probably, for NSNumber)

 /// BuildObjCNumericLiteral - builds an ObjCBoxedExpr AST node for the
 /// numeric literal expression. Type of the expression will be "NSNumber *”.

>> Comment need be updated.

Got it. Will update.

 ExprResult Sema::BuildObjCNumericLiteral(SourceLocation AtLoc, Expr *Number) {
@@ -456,10 +536,55 @@ ExprResult Sema::BuildObjCBoxedExpr(SourceRange SR, Expr *ValueExpr) {
   }
   ValueExpr = RValue.get();
   QualType ValueType(ValueExpr->getType());
-  if (const PointerType *PT = ValueType->getAs<PointerType>()) {
-    QualType PointeeType = PT->getPointeeType();
-    if (Context.hasSameUnqualifiedType(PointeeType, Context.CharTy)) {
+  if (ValueType->isBuiltinType() || ValueType->isEnumeralType()) {

>> You are adding an extra check for isEnumeralType() beyond the original source. Please add
>> comment for it and add test to back up the comment.

Previously there was another if branch for Enums, so code looked like

if char pointer
  handle NSString
else if builtin type
  handle NSNumber
else if enum
  handle NSNumber

I just merged the two NSNumber related branches.
But, yes, I will update comment and will try to add test.

-      
+

>> Please remove these diffs

Got it. It was caused by `git diff --check ...`

+// OS X Specific
++ (NSValue *)valueWithPoint:(NSPoint)point;
++ (NSValue *)valueWithSize:(NSSize)size;
++ (NSValue *)valueWithRect:(NSRect)rect;

>> It is better to add macosx availability attribute here.

Makes sense, will update.

+// iOS Specific
++ (NSValue *)valueWithCGPoint:(CGPoint)point;
++ (NSValue *)valueWithCGSize:(CGSize)size;
++ (NSValue *)valueWithCGRect:(CGRect)rect;
+

>> It is better to add iOS availability attribute here.

Got it.

+// 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

>> If you are not using ‘weak’ objects no need to provide -fobjc-runtime-has-weak 

Copy-paste >_<'
Will update.

>> Need one for iOS with its own checks.

Got it. Will update.

+// OS X Specific
+// CHECK:      [[METH:@.*]]         = private global{{.*}}valueWithPoint:{{.*}}
+// CHECK-NEXT: [[POINT_SEL:@.*]]    = {{.*}}[[METH]]{{.*}}
+// CHECK:      [[METH:@.*]]         = private global{{.*}}valueWithSize:{{.*}}
+// CHECK-NEXT: [[SIZE_SEL:@.*]]     = {{.*}}[[METH]]{{.*}}
+// CHECK:      [[METH:@.*]]         = private global{{.*}}valueWithRect:{{.*}}
+// CHECK-NEXT: [[RECT_SEL:@.*]]     = {{.*}}[[METH]]{{.*}}

>> No. Please have two distinct runs and checks for iOS and macosx. 

Got it. Will update.

+
+// iOS Specfific
+// CHECK:      [[METH:@.*]]         = private global{{.*}}valueWithCGPoint:{{.*}}
+// CHECK-NEXT: [[CGPOINT_SEL:@.*]]  = {{.*}}[[METH]]{{.*}}
+// CHECK:      [[METH:@.*]]         = private global{{.*}}valueWithCGSize:{{.*}}
+// CHECK-NEXT: [[CGSIZE_SEL:@.*]]   = {{.*}}[[METH]]{{.*}}
+// CHECK:      [[METH:@.*]]         = private global{{.*}}valueWithCGRect:{{.*}}
+// CHECK-NEXT: [[CGRECT_SEL:@.*]]   = {{.*}}[[METH]]{{.*}}
+

>> No. Please have two distinct runs and checks for iOS and macosx. 

Got it. Will update.

>> Sema tests should check for every erroneous situations. Like when NSValue is not available or does not have definition.
>> Or when APIs is not available for iOS or Macosx. 
>> Also need a test for __has_feature.

Got it. Will add more tests.

-- 
AlexDenisov
Software Engineer, https://github.com/AlexDenisov

On 8 Dec 2014 at 21:11:55, jahanian (fjahanian at apple.com) wrote:

Overall looks very nice. Some comments below. (I have tried to make them easier to find by prepending them with >>).

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20141209/d5d9f107/attachment.html>