> On Jun 10, 2015, at 2:26 PM, AlexDenisov <[email protected]> wrote: >> 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’.
Yeah, it does seem like a good refinement to the feature. You’ll need to add another method to ASTMutationListener; just look at previous patches that touch that interface to see what to do. >> 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. Thanks. > >> 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. It’s working because you’re allowing it to apply to typedefs. John. _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
