[Patch][ObjC][Proposal] NSValue literals

[AlexDenisov]

> On 25 Jun 2015, at 00:58, John McCall <rjmccall at apple.com> wrote:
> 
>> On Jun 24, 2015, at 3:12 PM, AlexDenisov <1101.debian at gmail.com> wrote:
>>> On 22 Jun 2015, at 23:20, John McCall <rjmccall at apple.com> wrote:
>>> 
>>>> On Jun 22, 2015, at 2:16 PM, AlexDenisov <1101.debian at gmail.com> wrote:
>>>>> On 22 Jun 2015, at 22:54, John McCall <rjmccall at apple.com> wrote:
>>>>> 
>>>>>> 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.
>>>> 
>>>> Just double-checked it: it has the same subject list.
>>> 
>>> Weird.  You should try to debug where Sema is emitting this diagnostic, then.
>> 
>> Yes, it was my bad. I applied attribute to a wrong declaration, now it’s fixed.
>> Also, I think we can easily drop support of typedef, because it works only if attribute applied after typedef, e.g.:
>> typedef old new __attribute(objc_boxable);
>> 
>> which is not a preferred option.
> 
> Okay.
> 
>>>>>>> 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*.
>>>> 
>>>> Actually I tried this initially, but clang complains about wrong types, e.g.:
>>>> sending 'NSPoint' (aka 'struct _NSPoint') to parameter of incompatible type 'const void *’
>>>> so I decided to wrap the ValueExpr with a ImplicitCastExpr.
>>> 
>>> Don’t ask PerformCopyInitialization to initialize the boxing method parameter; ask it to initialize a temporary of the record type.
>> 
>> Also done with this, though I’m not sure if I did it in a correct way.
>> 
>> Please, take a look at the new version.
> 
> +
> +  /// \bried An attribute was added to a RecordDecl
> 
> Typo: \brief.

Fixed.

> 
> +    // Emit CodeGen for first parameter
> +    // and cast value to correct type
> +    RValue RV = EmitAnyExprToTemp(SubExpr);
> 
> Hmm.  A better way to do this is:
> 
>  llvm::Value *Temporary = CreateMemTemp(SubExpr->getType());
>  EmitAnyExprToMem(SubExpr, Temporary, Qualifiers(), /*isInit*/ true);
> 

Done.

> It works out the same right now, but it expressed the intent better, which
> is that the expression needs to be evaluated into memory.
> 
> +    // Create char array to store type encoding
> +    std::string Str;
> +    getContext().getObjCEncodingForType(ValueType, Str);
> +    llvm::GlobalVariable *GV = CGM.GetAddrOfConstantCString(Str);
> +
> +    // Cast type enconding to correct type
> 
> Typo: encoding.

Fixed.

> 
> +    const ParmVarDecl *ArgDecl = BoxingMethod->parameters()[1];
> +    QualType ArgTy = ArgDecl->getType().getUnqualifiedType();
> +    llvm::Value *Cast = Builder.CreateBitCast(GV, ConvertType(ArgTy));
> 
> Minor nitpick: please come up with more descriptive variable names.
> In particular, you have two variables called “argDecl” and “ArgDecl”,
> and two variables called “ArgQT” and “ArgTy”!  I’d name the variables
> that apply to the second parameter “Encoding*”, like “EncodingArgDecl”
> or “EncodingArgTy”.
> 
> --- lib/Frontend/MultiplexConsumer.cpp
> +++ lib/Frontend/MultiplexConsumer.cpp
> @@ -14,6 +14,7 @@
> //===----------------------------------------------------------------------===//
> 
> #include "clang/Frontend/MultiplexConsumer.h"
> +#include "clang/AST/Attr.h"
> 
> You probably don’t actually need this #include.

True, fixed.

> 
> +    if (getLangOpts().CPlusPlus && ValueExpr->isGLValue()) {
> +      ExprResult Temp = PerformCopyInitialization(
> +                            InitializedEntity::InitializeTemporary(ValueType),
> +                            ValueExpr->getExprLoc(), ValueExpr);
> +      if (Temp.isInvalid())
> +        return ExprError();
> +      ValueExpr = Temp.get();
> +    }
> 
> I think you already do this later in the function.

Yes, noticed this as well, but forgot to drop.

> 
> +
> +    case UPD_ADDED_ATTR_TO_RECORD:
> +      AttrVec Attrs;
> +      Reader.ReadAttributes(F, Attrs, Record, Idx);
> +      D->setAttrsImpl(Attrs, Reader.getContext());
> +      break;
> 
> This replaces the attributes on the decl, which isn’t okay.
> If you need to add API to Decl to add a single attribute without calling
> getASTContext(), that’s fine.

It’s already there: void addAttr(Attr *A);
Fixed as well.

> +++ test/SemaObjCXX/objc-boxed-expressions-nsvalue.mm
> 
> Please add a couple of template-instantiation tests in this file.  Generally, you should test both an expression that’s dependent and an expression that isn’t.  That is, something like this:
> 
> template <class T> id box(T value) { return @(value); }
> void test_template_1(NSRect rect, NonTriviallyCopyable ntc) {
>  id x = box(rect);
>  id y = box(ntc);
> }
> 
> template <unsigned i> id boxRect(NSRect rect) { return @(rect); }
> template <unsigned i> id boxNTC(NonTriviallyCopyable ntc) { return @(ntc); }
> void test_template_2(NSRect rect, NonTriviallyCopyable ntc) {
>  id x = boxRect<0>(rect);
>  id y = boxNTC<0>(ntc);
> }
> 

Added these tests as well.

> Otherwise this is looking really good, thanks!
> 
> John.

-------------- 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/20150625/35ee0729/attachment.sig>