[Patch][ObjC][Proposal] NSValue literals

[John McCall]

> On Jun 10, 2015, at 2:26 PM, AlexDenisov <1101.debian at gmail.com> wrote:
>> I don’t think you meant to include this change.
> 
> Yep, wrong diff.
> 
>> You haven’t changed serialization to record this bit, but fortunately there’s
>> no need to; just remove it.  It’s completely acceptable to just test for the
>> attribute with hasAttr<ObjCBoxableAttr>().
> 
> Yes, I will, didn’t know about this API.
> 
>> SmallVectors aren’t allowed in the AST; they leak memory because we don’t run
>> destructors on AST nodes.  Fortunately, it’s not necessary; it’s straightforward to
>> change IRGen to directly generate the @encode value when the boxing method
>> takes two arguments.
>> 
>> You’ll want to change GetAddrOfConstantStringFromObjCEncode to take the
>> type instead of the ObjCEncodeExpr.
> 
> Yep, it makes sense, will stick to this approach.
> 
>> Testing for struct is unnecessary; a union is theoretically fine.
> 
> Roger that.
> 
>> This potentially changes earlier declarations, which requires more logic to
>> handle correctly — you'll need to create an AST update record if you want
>> to do this.
>> 
>> I recommend just handling the case where the target declaration is a RecordDecl.
> 
> Hm, initial intention was to give an ability to ‘enable’ this feature for legacy/third-party code, when a user doesn't have access to a definition.
> I guess, I'd need to create that ‘AST update record’.

Yeah, it does seem like a good refinement to the feature.  You’ll need to add another method to ASTMutationListener; just look at previous patches that touch that interface to see what to do.

>> I’m not sure why you needed to restructure this code instead of just adding
>> another case testing for RecordType after the EnumType case.
> 
> 'Historical reasons’: one of the previous versions supported also a pointer type, but was dropped at some point.
> I will revert this change.

Thanks.

> 
>> The attribute has to be written after the “struct” keyword to apply to the struct.
> 
> Current implementation is also working, though I don’t really know what kind of semantic difference it has. Anyway, will update if makes more sense and/or more correct.

It’s working because you’re allowing it to apply to typedefs.

John.