> On 25 Jun 2015, at 00:58, John McCall <[email protected]> wrote: > >> On Jun 24, 2015, at 3:12 PM, AlexDenisov <[email protected]> wrote: >>> On 22 Jun 2015, at 23:20, John McCall <[email protected]> wrote: >>> >>>> On Jun 22, 2015, at 2:16 PM, AlexDenisov <[email protected]> wrote: >>>>> On 22 Jun 2015, at 22:54, John McCall <[email protected]> wrote: >>>>> >>>>>> On Jun 21, 2015, at 2:22 PM, AlexDenisov <[email protected]> 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.
signature.asc
Description: Message signed with OpenPGP using GPGMail
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
