[libcxx-commits] [PATCH] D148432: [libc++] Simplify the tuple constructor overload set

Mon May 8 08:19:36 PDT 2023

ldionne accepted this revision as: ldionne.
ldionne added a subscriber: aaron.ballman.
ldionne added a comment.

I really like this. It's a huge simplification. If it really can't be noticed by users (and it can't AFAICT), we're literally not losing anything and we're gaining a ton of readability and maintainability.

Nikolas also mentions:

1. Diagnostics are much better
2. Fewer notes are generated when an error occurs
3. The overload set is much smaller

Overall, I think this is great. Like I mentioned on https://reviews.llvm.org/D148431#4323187, I think we should be careful with enabling those extensions widely just in case we notice an issue with doing that. We wouldn't want to be stuck in a place where we use a bunch of extensions that are really hard to revert but we realize there's a problem with using those.

Hence, I'd suggest that we only make this change before the LLVM 17 release. Then we can release with this extension (and only this one) and gain additional information. I think we will most likely not learn anything new, in which case we would conclude that using those extensions truly is not noticeable and doesn't hurt anyone, and we could apply them more widely. But if we learn something new, at least we'll have a single patch to revert instead of potentially many patches in various parts of the code.

CCing libc++ vendors for awareness and also @aaron.ballman since we had a discussion adjacent to this topic on Discord. If nobody raises a substantiated objection by the end of this week, I'd ship this.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148432