[Patch][ObjC][Proposal] NSValue literals

[John McCall]

> On Jun 16, 2015, at 1:53 PM, AlexDenisov <1101.debian at gmail.com> wrote:
> 
> Seems I’ve addressed and fixed all the comments/remarks.
> ATM I have another issue: I added test for objc_boxable into the test/PCH/subscripting-literals.m and now clang crashes, I’m still trying to find and fix the problem, but decided to send the new patch version so you can check if I’m on the right way.
> 
> P.S. re: crashing test
> after de-serialization something goes wrong with the second parameter of ObjCBoxedExpr  and valueWithBytes:objCType: and clang segfaults.
> I also attach the stack-trace of the crash if you, by any chance, want to take a look and give me some hint/direction to dig.

+    #if __has_attribute(objc_boxable)
+        typedef struct _Rect Rect __attribute__((objc_boxable));
+    #endif
+

Please put the attribute in the correct position (after “struct”) in this example as well.  Examples should consistently emphasize the correct position, just so that people learn it for this and other attributes.

+  /// \brief The declaration of the Objective-C NSValue class.
+  ObjCInterfaceDecl *NSValueDecl;
+

+  /// \brief The declaration of the valueWithBytes:objCType: method.
+  ObjCMethodDecl *ValueWithBytesObjCTypeMethod;
+

I don’t know if it’s causing your crash, but it looks like you never zero-initialize these in the Sema constructor.

+bool Type::isObjCBoxableStructureType() const {

We should probably call this isObjCBoxableRecordType() to eliminate any confusion.

+  // ObjCBoxedExpr supports boxing of structs and unions 
+  // via [NSValue valueWithBytes:objCType:]
+  if (const UnaryOperator *UO = 
+                              dyn_cast<UnaryOperator>(SubExpr->IgnoreCasts())) {
+    QualType ValueType(UO->getSubExpr()->IgnoreCasts()->getType());
+    if (ValueType->isObjCBoxableStructureType()) {

I don’t understand the purpose of checking for a UnaryOperator here.  Can you not just look at the type?

+static void handleObjCBoxable(Sema &S, Decl *D, const AttributeList &Attr) {
+  RecordDecl *RD = nullptr;
+  if (const TypedefDecl *TD = dyn_cast<TypedefDecl>(D)) {
+    const RecordType *RT = TD->getUnderlyingType()->getAs<RecordType>();
+    RD = RT->getDecl();
+  } else {
+    RD = dyn_cast<RecordDecl>(D);
+  }
+  if (RD) {
+    ObjCBoxableAttr *BoxableAttr = ::new (S.Context) 
+                          ObjCBoxableAttr(Attr.getRange(), S.Context,
+                                          Attr.getAttributeSpellingListIndex());
+    RD->addAttr(BoxableAttr);
+    if (ASTMutationListener *L = S.getASTMutationListener())
+      L->AddedAttributeToRecord(BoxableAttr, RD);
+  }
+}

Please only notify the ASTMutationListener when adding this to a record found via a typedef, and please add a comment explaining what’s going on here.

+  } else if (ValueType->isObjCBoxableStructureType()) {
+    // Support for structure types, that marked as objc_boxable
+    // struct __attribute__((objc_boxable)) s { ... };
+    
+    // Look up the NSValue class, if we haven't done so already. It's cached
+    // in the Sema instance.
+    if (!NSValueDecl) {

Some of your comments in this block still talk about NSString.

Is there some way we can share some of the code between these cases?

+  
+  /// \bried An attribute was added to a RecordDecl
+  ///
+  /// \param Attr The attribute that was added to the Record
+  ///
+  /// \param Record The RecordDecl that got a new attribute
+  virtual void AddedAttributeToRecord(const Attr *Attr, 
+                                      const RecordDecl *Record) {}

This is the right start, but it looks like you didn’t update the ASTWriter.  You’ll need to add a new AST update kind like UPD_CXX_DEDUCED_RETURN_TYPE; that’s a good example, look at the code for that in both ASTWriter and ASTReader.

John.