On Tue, Dec 12, 2017 at 1:45 PM, David Blaikie <dblai...@gmail.com> wrote:
> On Mon, Dec 11, 2017 at 5:38 PM John McCall <rjmcc...@gmail.com> wrote: > >> On Mon, Dec 11, 2017 at 6:19 PM, David Blaikie <dblai...@gmail.com> >> wrote: >> >>> On Mon, Dec 11, 2017 at 3:16 PM John McCall via Phabricator < >>> revi...@reviews.llvm.org> wrote: >>> >>>> rjmccall added a comment. >>>> >>>> In https://reviews.llvm.org/D41039#951648, @ahatanak wrote: >>>> >>>> > I had a discussion with Duncan today and he pointed out that perhaps >>>> we shouldn't allow users to annotate a struct with "trivial_abi" if one of >>>> its subobjects is non-trivial and is not annotated with "trivial_abi" since >>>> that gives users too much power. >>>> > >>>> > Should we error out or drop "trivial_abi" from struct Outer when the >>>> following code is compiled? >>>> > >>>> > struct Inner1 { >>>> > ~Inner1(); // non-trivial >>>> > int x; >>>> > }; >>>> > >>>> > struct __attribute__((trivial_abi)) Outer { >>>> > ~Outer(); >>>> > Inner1 x; >>>> > }; >>>> > >>>> > >>>> > The current patch doesn't error out or drop the attribute, but the >>>> patch would probably be much simpler if we didn't allow it. >>>> >>>> >>>> I think it makes sense to emit an error if there is provably a >>>> non-trivial-ABI component. However, for class temploids I think that >>>> diagnostic should only fire on the definition, not on instantiations; for >>>> example: >>>> >>>> template <class T> struct __attribute__((trivial_abi)) holder { >>>> T value; >>>> ~holder() {} >>>> }; >>>> holder<std::string> hs; // this instantiation should be legal despite >>>> the fact that holder<std::string> cannot be trivial-ABI. >>>> >>>> But we should still be able to emit the diagnostic in template >>>> definitions, e.g.: >>>> >>>> template <class T> struct __attribute__((trivial_abi)) named_holder { >>>> std::string name; // there are no instantiations of this template >>>> that could ever be trivial-ABI >>>> T value; >>>> ~named_holder() {} >>>> }; >>>> >>>> The wording should be something akin to the standard template rule that >>>> a template is ill-formed if it has no valid instantiations, no diagnostic >>>> required. >>>> >>>> I would definitely like to open the conversation about the name of the >>>> attribute. I don't think we've used "abi" in an existing attribute name; >>>> usually it's more descriptive. And "trivial" is a weighty word in the >>>> standard. I'm not sure I have a great counter-proposal off the top of my >>>> head, though. >>>> >>> >>> Agreed on both counts (would love a better name, don't have any >>> stand-out candidates off the top of my head). >>> >>> I feel like a more descriptive term about the property of the object >>> would make me happier - something like "address_independent_identity" >>> (s/identity/value/?) though, yeah, that's not spectacular by any stretch. >>> >> >> Incidentally, your comments are not showing up on Phabricator for some >> reason. >> > > Yeah, Phab doesn't do a great job looking on the mailing list for > interesting replies - I forget, maybe it only catches bottom post or top > post but not infix replies or something... > Well, fortunately the mailing list is also archived. :) > The term "trivially movable" suggests itself, with two caveats: >> - What we're talking about is trivial *destructive* movability, i.e. >> that the combination of moving the value to a new object and not destroying >> the old object can be done trivially, which is not quite the same as >> trivial movability in the normal C++ sense, which I guess could be a >> property that someone theoretically might care about (if the type is >> trivially destructed, but it isn't copyable for semantic reasons?). >> - Trivial destructive movability is a really common property, and it's >> something that a compiler would really like to optimize based on even in >> cases where trivial_abi can't be adopted for binary-compatibility reasons. >> Stealing the term for the stronger property that the type is trivially >> destructively movable *and its ABI should reflect that in a specific way* >> would be unfortunate. >> > > Fair point. > > >> "trivially_movable" is a long attribute anyway, and >> "trivially_destructively_movable" is even worse. >> >> Maybe that second point is telling us that this isn't purely descriptive >> — it's inherently talking about the ABI, not just the semantics of the >> type. I might be talking myself into accepting trivial_abi if we don't end >> up with a better suggestion. >> > > *nod* I think if you want to slice this that way (ensuring there's space > to support describing a similar property for use only in non-ABI-breaking > contexts as well) it seems like ABI is the term to use somewhere in the > name. > Yeah. We just had a little internal conversation about it, and the idea of "address_invariant_abi" came up, but I'm not sure it has enough to recommend it over "trivial_abi" to justify the longer name. > Random thing that occurred to me: is it actually reasonable to enforce >> trivial_abi correctness in a non-template context? Templates aren't the >> only case where a user could reasonably want to add trivial_abi and just >> have it be suppressed if it's wrong. Imagine if some stdlib made >> std::string trivial_abi; someone might reasonably want to make my >> named_holder example above trivial_abi as well, with the expectation that >> it would only have an effect on targets where std::string was trivial_abi. >> At the very least, I'm concerned that we might be opening ourselves up to a >> need to add supporting features, like a way to be conditionally trivial_abi >> based on context. >> > > Fair point, much like the quirky but useful behavior of "= default". Good > point about non-dependent contexts still being relevant to this subjective > behavior... > > I was already leaning towards this being a warning rather than an error - > this situation leans me moreso that way & possibly suppressing the warning > when the types are split between system and non-system headers (if the > attribute's on a type in a non-system header, but the type that's blocking > it from being active is in a system header, don't warn). > That's an interesting idea. John.
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
