[libcxx-commits] [PATCH] D96523: [libc++] Rewrite the tuple constructors to be strictly Standards conforming

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver added inline comments.


================
Comment at: libcxx/include/tuple:479
+    _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR
+    explicit tuple()
+        _NOEXCEPT_(_And<is_nothrow_default_constructible<_Tp>...>::value)
----------------
ldionne wrote:
> zoecarver wrote:
> > ldionne wrote:
> > > zoecarver wrote:
> > > > When were these conditionally explicit overloads added? I think C++17. 
> > > > 
> > > > This is a bit annoying because in C++20 we get the `explicit(bool)` operator which would make this implementation about 50% smaller. 
> > > > 
> > > > It looks like that was added to clang in V10. I wonder if we could get away with a macro to do this. There would be a small overlap of users on the V6-V10 clang version using C++17 who might be affected. 
> > > > When were these conditionally explicit overloads added? I think C++17.
> > > 
> > > Hmm, I'm not 100% sure, but that sounds about right. Perhaps we've been supporting those in older Standards as an extension. While I generally dislike extensions, I think that not enabling those implicit/explicit constructors pre-C++17 would be a pretty big cost in complexity for little benefit, so I'd rather implement them even pre-C++17. Open to discussion, of course.
> > > 
> > > > This is a bit annoying because in C++20 we get the explicit(bool) operator
> > > 
> > > Yes, I agree, those are a huge pain to write without `explicit(bool)`, but I don't think there's a way around that TBH. To be fair, there is quite a bit of code duplication, but it's simple code. I was also planning on simplifying the calls to the `__base_` constructor in a follow-up patch to reduce the magnitude of the duplication. Let me know if you have a blocking comment or if you want me to take an action here.
> > > While I generally dislike extensions, I think that not enabling those implicit/explicit constructors pre-C++17 would be a pretty big cost in complexity for little benefit, so I'd rather implement them even pre-C++17.
> > 
> > I think if we removed the extension, we'd actually reduce the complexity significantly. Here's what I had in mind:
> > ```
> > #if STD > 14 && CLANG >=10
> > #define _EXPLICIT_COND(...) explicit(...)
> > #else
> > #define _EXPLICIT_COND
> > #endif
> > ```
> > Then we could just annotate all these overloads with `_EXPLICIT_COND(...)`. The problem is we'd be non-conforming for those using C++17 on clang V9 or lower, and this would potentially break people's code if they were on C++11 or 14. WDYT? Could we get away with it? 😉
> > 
> > This is a non-blocking comment. I just thought it might be a way to reduce a bunch of code (but it looks it probably won't work for now, but maybe we can come back to it in a few years).
> Does Clang trunk provide `explicit(bool)` as an extension in C++ < 20 modes? If so, then I suggest we come back once we drop support for older Clangs and remove the duplication.
> 
> Otherwise, if Clang doesn't support `explicit(bool)` in pre-C++20 mode, we will never be able to remove this duplication because we still have to implement the explicit/implicit constructors for tuple pre-C++20 (it was applied as a LWG issue IIRC).
[[ https://godbolt.org/z/jfYvjx | Yes. ]] If there were a world where libc++ only supported Clang V10 and GCC V9, I think this could work really well. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96523