> 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. >>> 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. Are there any robust ways to avoid this warning? (Excerpt of patching the validation method if ValueExpr type is objc_boxable) > >>> 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. Ohh, I wish I could spend the time learning operators precedence than debugging wrong stuff… Thank you, John! > > 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
