[PATCH] D99834: Don't check that std::pair is trivially copyable on FreeBSD

[Louis Dionne via Phabricator via llvm-commits]

ldionne added inline comments.


================
Comment at: llvm/unittests/Support/TypeTraitsTest.cpp:113
   TrivialityTester<std::function<int()>, false, false>();
+#if !defined(_LIBCPP_DEPRECATED_ABI_DISABLE_PAIR_TRIVIAL_COPY_CTOR)
   TrivialityTester<std::pair<int, bool>, true, true>();
----------------
dim wrote:
> ldionne wrote:
> > dim wrote:
> > > ldionne wrote:
> > > > I'm really not fond of using a libc++ internal macro here. I think we should instead detect that we're on FreeBSD.
> > > At the moment in `__config` the part where this macro gets defined is indeed only when `__FreeBSD__` is defined:
> > > 
> > > ```
> > > // Feature macros for disabling pre ABI v1 features. All of these options
> > > // are deprecated.
> > > #  if defined(__FreeBSD__)
> > > #    define _LIBCPP_DEPRECATED_ABI_DISABLE_PAIR_TRIVIAL_COPY_CTOR
> > > #  endif
> > > #endif
> > > ```
> > > 
> > > So this could as well be `#if !defined(__FreeBSD__)`, yes. At some point though we'll have to migrate to libc++ ABI v2 (whenever that comes), so then this check would have to be adjusted too...
> > > 
> > Then it can be adjusted if you want to move to a different ABI based on some other detection. I'm very very uncomfortable with letting people use implementation details of libc++, that is the source of huge headache for us.
> Even though this is all within the llvm project, I get your point. It becomes hard to rename that macro once this gets duplicated somehow. So let's change this test to `#if !defined(__FreeBSD__)` for now.
> 
> (That said, it's still an annoying wart in FreeBSD that we have the sort-of-pre-v1 ABI that has the non-trivial pair constructor. However we'd have to keep a compat lib in place if we ever want to change the ABI, so I'd rather wait until the libc++ v2 ABI is mostly finalized. Users of the old ABI can then install a compat libc++.so.1 binary.)
Thank you!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99834/new/

https://reviews.llvm.org/D99834