> 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. >> 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*. >> 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. John. _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
