[PATCH] D54472: Disable invalid isPodLike<> specialization

[serge via Phabricator via llvm-commits]

serge-sans-paille added inline comments.


================
Comment at: include/llvm/Support/type_traits.h:40-44
+#define LLVM_VERIFY_ISPODLIKE_CONSISTENCY(T)                                   \
+  static_assert(llvm::isPodLike<T>::value ==                                   \
+                    std::is_trivially_copyable<T>::value,                      \
+                "pod-like implies trivially copyable")
+
----------------
dblaikie wrote:
> serge-sans-paille wrote:
> > dblaikie wrote:
> > > Does the presence of this macro mean "no one should ever specialize isPodLike"? & if they don't, then its certain that isPodLike == is_trivially_copyable, by definition?
> > > 
> > > If so, then I'm not sure of the importance of the macro - we should just say "don't specialize isPodLike, ever & leave it at that, maybe?
> > Almost. It means « if you specialize ``isPodLike``, then make sure that for modern compiler, it does not conflict with std::is_trivially_copyable ».
> > 
> > Basically for modern compiler, we should have
> > 
> > ``template<class T> using isPodLike = std::is_trivially_copiable<T>;``
> > 
> > However, some compilers supported by LLVM don't implement that feature. In my opinion, it is still valid to speicalize ``isPodLike`` to ensure decent performance in these case. But not at the cost of UB.
> OK, fair enough. 
> 
> Though does the performance matter on platforms that don't support the real trait? Which compilers/versions is that compared to the set of supported compilers/versions for building Clang/LLVM. (also, in part, the performance of Clang/LLVM is most important on a self-host build - correctness more broadly, but those interest in performance should likely be bootstrapping)
> 
> In any case, /if/ we're going to keep that performance feature (whereby LLVM developers can annotate types they know would pass is_trivially_copyable if it were available), probably validating its usage should be kept at the same place as the trait specializations, rather than in the usage?
> 
> Any chance of improving/changing Optional/SuccIterator to be trivially copyable when their parameters are trivially copyable?
> 
> (& is libc++'s std::pair trivially copyable when T1 and T2 are trivially copyable?)
> probably validating its usage should be kept at the same place as the trait specializations, rather than in the usage?

The only solutipon I have for that is to define `struct isPodLike : details::isPodLike<T>`, perform the check in `isPodLike` and have user specialize `details::isPodLike` but nothing would prevent them from directly specializing `isPodLike` so I'm not a big fan of this soltion.

My best catch would be to define `isPodLike` as a template alias, that would make it non-speicalizable, no need for extra check and (slightly?) degraded performance for legacy compilers. I have the patch ready for that.

> Any chance of improving/changing Optional/SuccIterator to be trivially copyable when their parameters are trivially copyable?

Nop, they overload the assign operator, which need to be defaulted :-/
In the case of Optionnal, I may give it a try, but probably in another commit.

Can't do anything for std::pair :-)


https://reviews.llvm.org/D54472