[Patch][ObjC][Proposal] NSValue literals

[John McCall]

> On Jun 21, 2015, at 2:22 PM, AlexDenisov <1101.debian at gmail.com> wrote:
>> 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.

This probably means that your entry in Attr.td has the wrong subject list; it should use the same Subjects line as ObjCBridge.

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

Hmm.  This sounds like it's unnecessary residue of the old AST pattern, where you needed the first operand to be a const void* because that was the type of the parameter.  Just leave the operand as an r-value of the record type, have IR-gen emit the expression into a temporary, and cast the address of that temporary to i8*.

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

Sounds good.

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

Oh, you’re making an iterator mistake; you need to be doing *(BoxingMethod->param_begin() + 1), or better yet, BoxingMethod->parameters()[1].

The only reason it’s working in unserialized code is that the parser isn't allocating anything between the two ParmVarDecls.

John.