[Patch][ObjC][Proposal] NSValue literals

[AlexDenisov]

> 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.

Clang complains that attribute cannot be applied to a struct declaration, which means that attribute at the end of the type definition is correct.
It is possible to allow using of objc_boxable with a struct declaration, but in this case using of a typedef doesn’t make any sense.

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

It doesn’t the problem, though it’s a nice catch.

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

Done. Also added test for a union, to be fair.

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

When ObjCBoxedExpr creates with a record (struct/union) then the lvalue should be casted to ‘const void *’, which leads to a UnaryOperator here. Anyway, I agree that such type-casting looks weird.
Currently, I rewrote it and I still check the canonical type. I can move this check into ObjCBoxedExpr, but I need the underlaying type to generate correct encoding (@encode(whatnot)).

> 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.

Done.

> 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?

Well, it’s obviously a copypasta. I also don’t like this part and want to improve it, but the fix will involve changing of build-ObjCDictionary/ObjCArray-literal methods, which is out of scope of this patch. I’d like to suggest another patch with this particular fix when we finish this one.

> 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.

It was a bit tricky, but it’s done now. Also, I added a test for this case.


re: crash

lib/CodeGen/CGObjC.cpp:
+    const ParmVarDecl *ArgDecl = *BoxingMethod->param_begin() + 1;
+    QualType ArgTy = ArgDecl->getType();

Something is going wrong here with the ArgTy only after deserialization, so I don’t really know where the bug is - ASTWriter or ASTReader. I played around with a debugger and sixth sense, but didn’t find the root of the problem yet - serialization algorithm still a bit cryptic for me.

Also, I tried to use a hardcoded type, e.g.:

QualType ArgTy = Context.getPointer(Context.CharTy.withConst());

and it works just fine.
It can be an acceptable solution, since we expect the second parameter of valueWithBytes:objCType: to have type ‘const char *’, but in fact it’s just unfair dirty trick and a workaround, that hides the bug…

Best regards,
Alex.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: objc_boxable.patch
Type: application/octet-stream
Size: 68212 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150621/99dbfd52/attachment.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 496 bytes
Desc: Message signed with OpenPGP using GPGMail
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150621/99dbfd52/attachment.sig>