[Patch][ObjC][Proposal] NSValue literals

[AlexDenisov]

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

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

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

> There are some extra things you need to do here to do the right thing in
> ObjC++.  (In principle, it would be better to check them when applying the
> attribute, but we can’t rely on the declaration being complete at that point.)
> 
> The first is that DefaultFunctionArrayLvalueConversion does not actually
> convert l-values of struct/union type to r-values in C++.  You’ll need to do this:
> 
> if (getLangOpts().CPlusPlus && ValueExpr->isGLValue()) {
>   ExprResult Temp = PerformCopyInitialization(
>                      InitializedEntity::InitializeTemporary(ValueExpr->getType()),
>                                               ValueExpr->getExprLoc(), ValueExpr);
>   if (Temp.isInvalid())
>     return ExprError();
>   ValueExpr = Temp.get();
> }
> 
> That will also implicitly check that the type is complete.
> 
> The second is that you should test whether the type is trivially copyable
> (with ValueType->isTriviallyCopyableType(Context)) and diagnose if it isn’t.

Got it, will update as well.


Thank you for feedback,
Alex.

-------------- 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/20150610/5140994e/attachment.sig>