devin.jeanpierre added a comment. In D114732#3256046 <https://reviews.llvm.org/D114732#3256046>, @devin.jeanpierre wrote:
> Sorry, I missed your other comments. Let me know if there's anything else I > didn't address. I just noticed that there's a "Done" checkbox next to each comment, and it isn't automatically checked if I reply to it -- I suppose I should be checking these so that it's easier to keep track of what's left to do? Sorry, this is my first time using phabricator! ================ Comment at: clang/test/SemaCXX/attr-trivial-abi.cpp:9-11 +// On Windows, trivial relocatability depends only on a trivial copy constructor existing. +// In this case, it is implicitly deleted. Similar concerns apply to later tests. +static_assert(!__is_trivially_relocatable(a<int>), ""); ---------------- Quuxplusone wrote: > devin.jeanpierre wrote: > > Quuxplusone wrote: > > > (1) Why should Windows be different from everyone else here? > > > (2) AFAIK, a user-defined move ctor means the copy ctor is //not > > > implicitly declared//, but it's not //deleted//; so I think this comment > > > is slightly wrong. > > > (2) AFAIK, a user-defined move ctor means the copy ctor is not implicitly > > > declared, but it's not deleted; so I think this comment is slightly wrong. > > Sorry, I get that confused a lot. :) > > > > > (1) Why should Windows be different from everyone else here? > > In this change so far, an object is only considered trivially relocatable > > if it's trivial for calls, and Windows is non-conforming wrt what it > > considers trivial for calls: it won't consider something trivial for calls > > if it isn't trivially copyable. > > > > Code: https://clang.llvm.org/doxygen/SemaDeclCXX_8cpp_source.html#l06558 > > > > It surprised me too. This is a good motivator IMO to support something like > > your proposal, and have types be trivially relocatable that aren't trivial > > for calls. This allows, as you mention elsewhere, for optimizations that > > aren't ABI-breaking, and for e.g. non-conforming platforms like this to > > still take advantage of trivial relocatability. > > In this change so far, an object is only considered trivially relocatable > > if it's trivial for calls, and Windows [has a calling convention that's > > different from the Itanium C++ ABI's calling convention]: it won't consider > > something trivial for calls if it isn't trivially copyable. > > Ah, I see, and I agree with your logic here. It's unintuitive but only for > the same reasons we've already hashed over: we're introducing an > `__is_trivially_relocatable(T)` that gives zero false-positives, but (for > now) frequent false-negatives, and this just happens to be one //additional// > source of false negatives on Win32 specifically. (And I keep tripping over it > because this frequent-false-negative `__is_trivially_relocatable(T)` is named > the same as D50119's "perfect" discriminator.) > > Everyone's on board with the idea that we're promising to preserve the true > positives forever, but at the same time we're expecting that we might > //someday// fix the false negatives, right? I.e., nobody's expecting > `!__is_trivially_relocatable(a<int>)` to remain true on Win32 forever? (I'm > pretty sure we're all on board with this.) > Everyone's on board with the idea that we're promising to preserve the true > positives forever, but at the same time we're expecting that we might someday > fix the false negatives, right? I.e., nobody's expecting > !__is_trivially_relocatable(a<int>) to remain true on Win32 forever? (I'm > pretty sure we're all on board with this.) +1 My hope is that future changes can fix the false negatives -- either with explicit annotations (like `[[trivially_relocatable]`), or with improved automatic inference, etc. (For example, we could imagine marking types as trivially relocatable if they have *a* trivial (for calls) copy or move constructor and a trivial (for calls) destructor, even if they also have a nontrivial move or copy destructor. This would be a superset of trivial-for-calls types on all platforms, and especially Windows.) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D114732/new/ https://reviews.llvm.org/D114732 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
