[Patch][ObjC][Proposal] NSValue literals

AlexDenisov 1101.debian at gmail.com
Sun Nov 23 08:09:35 PST 2014 

Finally, the problem is solved.
I’ve found that EmitObjCBoxedExpr uses EmitAnyExpr to handle SubExpr (which might be treated as a method call' argument), while EmitObjCMessageExpr uses EmitCallArgs.
To solve the problem I just unified a bit EmitObjCBoxedExpr, so now it also uses EmitCallArgs.

Here is the part of the full diff, which illustrates unification of EmitObjCBoxedExpr and EmitObjCMessageExpr:
diff --git a/include/clang/AST/ExprObjC.h b/include/clang/AST/ExprObjC.h
index 817c0cc..82738da 100644
--- a/include/clang/AST/ExprObjC.h
+++ b/include/clang/AST/ExprObjC.h
@@ -124,6 +124,15 @@ class ObjCBoxedExpr : public Expr {
    
   // Iterators
   child_range children() { return child_range(&SubExpr, &SubExpr+1); }
+  
+  typedef ConstExprIterator const_arg_iterator;
+   
+  const_arg_iterator arg_begin() const {
+    return reinterpret_cast<Stmt const * const*>(&SubExpr);
+  }
+  const_arg_iterator arg_end() const {
+    return reinterpret_cast<Stmt const * const*>(&SubExpr + 1);
+  }
    
   friend class ASTStmtReader;
 };
diff --git a/lib/CodeGen/CGObjC.cpp b/lib/CodeGen/CGObjC.cpp
index ca67c4b..d2975b8 100644
--- a/lib/CodeGen/CGObjC.cpp
+++ b/lib/CodeGen/CGObjC.cpp
@@ -55,12 +55,12 @@ llvm::Value *CodeGenFunction::EmitObjCStringLiteral(const ObjCStringLiteral *E)
 /// EmitObjCBoxedExpr - This routine generates code to call
 /// the appropriate expression boxing method. This will either be
-/// one of +[NSNumber numberWith<Type>:], or +[NSString stringWithUTF8String:].
+/// one of +[NSNumber numberWith<Type>:], or +[NSString stringWithUTF8String:],
+/// or [NSValue valueWith<Type>:].
 ///
 llvm::Value *
 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());
  
   RValue result = Runtime.GenerateMessageSend(
       *this, ReturnValueSlot(), BoxingMethod->getReturnType(), Sel, Receiver,

The following steps have to be done before final submission:

 - add tests for non-arc environment. I’m going to look for a better way to handle this, but any suggestions on best practices are welcome.
 - add full support for NSValue: 
	- valueWithEdgeInsets:(NSEdgeInsets), availability: OS X 10.10 and higher, iOS 8 and higher.
	- valueWithNonretainedObject:(id), availability: OS X, iOS
	- valueWithPointer:(const void *), availability: OS X, iOS
	- valueWithCATansform3D:(CATansform3D), availability: OSX, iOS
	- valueWithCGAffineTransform:(CGAffineTransform), availability: iOS
	- valueWithUIOffset:(UIOffset), availability: iOS 5.0 and higher
	- valueWithUIEdgeInsets:(UIEdgeInsets), availability: iOS
	- valueWithCGVector:(CGVector), availability: iOS

If I missed something or there any objections, please, let me know.
-- 
AlexDenisov
Software Engineer, https://github.com/AlexDenisov

On 22 Nov 2014 at 20:56:26, AlexDenisov (1101.debian at gmail.com) wrote:

Here is another patch, with tests for CodeGen, it covers the same data types: NS/CG Rect, Size, Point and NSRange.
I’ve compiled test file with calls like ‘NSValue valueWith<Type>:' into LLVM IR, then added checks into the test file, they all passed.
After that I replaced direct NSValue calls with boxed expressions: @(cg_point), @(ns_rect), etc.
All the tests passes, excerpt of two which use NS/CGRect.
For the both cases CGM generates %ns_rect (%cg_rect respectively) variable and fills it with some data.
In case of direct call (valueWithRect) it just passes the variable to objc_msgSend, but in case of boxed expression there are some extra steps.
CGM creates additional variable (%agg-temp), copies %ns_rect’s value into it and passes the new var into objc_msgSend.
Actually, generated program has correct behaviour, but it has extra variable, which means that something is wrong.

Currently, I have no idea what is wrong and how to fix the issue, but, definitely, I’m going to dive deeper into the CodeGenModule.
I would really appreciate any suggestions or advice where to start. Also, any feedback on newly added CG tests are welcome.

In attachment you can find the patch and diff, which explains my problem in a better way.

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

On 21 Nov 2014 at 18:10:46, AlexDenisov (1101.debian at gmail.com) wrote:

Great, so let’s talk later, with another patch.
Thank you for help!
-- 
AlexDenisov
Software Engineer, https://github.com/AlexDenisov

On 21 Nov 2014 at 17:28:18, jahanian (fjahanian at apple.com) wrote:


On Nov 21, 2014, at 4:45 AM, AlexDenisov <1101.debian at gmail.com> wrote:

- It is important to have a complete IRGen tests for all the supported boxed items.
Could you, please, point me to the right direction? I’ve found only few tests for literals under CodeGenObjC: boxing.m and objc-literal-tests.m. Should I put new one there as well or just create another file?


The best one to look at is arc-literals.m. And it is good to produce the test under -fobjc-arc and without. 
Generally, you need to compile your test with -emit-llvm -o -  and save the output and then put the CHECKS
around the code you care enough (and make the register references generic, etc.).
It will take several iterations to get it right. 
Since you need many tests of a variety of APIs it is good to have a brand new test.

- CGPoint, CGSize, CGRect are iOS APIs only. But I haven’t been able to find their APIs in
  the later version of iPhone SDKs. If they have been removed recently, then we should make sure to check
  the deployment target and issue proper diagnostics if need be. This is generally the case for all the APIs.
  If we are going to use auto boxing, then we need to make sure the APIs available in earlier (or later) versions
  of the SDKs.
CG-structs are part of the CoreGraphics.framework. On OS X NSValue' factory methods is a part of Foundation.framework (NSGeometry.h, NSValueGeometryExtensions), while on iOS they’re part of UIKit (UIGeometry.h, NSValueUIGeometryExtensions). I guess they’re not going to disappear. Bu, I totally agree - deployment target should be checked for methods/structs, which is unavailable at some target version (e.g. NSEdgeInsets, UIOffset).


Great thanks.

- Have you though of supporting other item types? 
  NSEdgeInsets (CGEdgeInsets for iOS?) macosx 10.10,  iPhone 8.0 
  non-retaining Objective-C objects.
  Any C pointers

I wasn’t sure if this feature will be accepted (and still don't), so decided to sent patch only for these structs. But, yes, in my opinion support for the whole ‘NSValueBoxable’ data types should be implemented.

OK.  While you are focusing on the structs these can wait.



Also, I've realized later, there should be support for modernizer to convert ‘old’ syntax to the new one.

Definitely, we should do that too. (I can help with that).

I will add more tests, and, probably add other data types support, if there are no objections.

I would wait on the other data type support until we have gone through the review process as it might impact implementation.


Thank you for feedback.

Thank you so much for working on this.
- Fariborz

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

On 20 Nov 2014 at 20:14:39, jahanian (fjahanian at apple.com) wrote:

While, the review process is ongoing, I looked at the patch. It is generally very good and closely follows the convention for
doing auto boxing of other types. I have few general comments to make.

- It is important to have a complete IRGen tests for all the supported boxed items.

- CGPoint, CGSize, CGRect are iOS APIs only. But I haven’t been able to find their APIs in
  the later version of iPhone SDKs. If they have been removed recently, then we should make sure to check
  the deployment target and issue proper diagnostics if need be. This is generally the case for all the APIs.
  If we are going to use auto boxing, then we need to make sure the APIs available in earlier (or later) versions
  of the SDKs.

- Have you though of supporting other item types? 
  NSEdgeInsets (CGEdgeInsets for iOS?) macosx 10.10,  iPhone 8.0 
  non-retaining Objective-C objects.
  Any C pointers

 One code comment: getNSValueFactoryMethod has two default arguments but do not have non-default value.
 
- Fariborz

On Nov 16, 2014, at 10:15 AM, AlexDenisov <1101.debian at gmail.com> wrote:

Patch extends boxing expressions to support NSValue.

Some C structures might be boxed into a NSValue, e.g.: NSPoint, CGPoint, NSRect, etc.
This patch extends boxing expressions to accept the structures, that could be used to construct NSValue object:

NSPoint p;
NSValue *point = @(p);
CGRect r;
NSValue rect = @(r);

Full list of supported structures:
NSPoint, NSSize, NSRect, CGPoint, CGSize, CGRect, NSRange.

-- 
AlexDenisov
Software Engineer, https://github.com/AlexDenisov
<nsvalue_literals.diff>_______________________________________________
cfe-commits mailing list
cfe-commits at cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

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

More information about the cfe-commits mailing list