[libcxx-commits] [PATCH] D131868: [libc++][NFC] Refactor enable_ifs in vector

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const requested changes to this revision.
var-const added a comment.
This revision now requires changes to proceed.

Can you please add the rationale for this change to the patch description? Other than that and the couple optional comments, LGTM.



================
Comment at: libcxx/include/vector:398-399
+  template <class _InputIterator,
+            __enable_if_t<__is_exactly_cpp17_input_iterator<_InputIterator>::value &&
+                              is_constructible<value_type, typename iterator_traits<_InputIterator>::reference>::value,
+                          int> = 0>
----------------
It looks like there are 2 same conditions repeated throughout the file:
```
__is_exactly_cpp17_input_iterator<_InputIterator>::value &&
is_constructible<value_type, typename iterator_traits<_InputIterator>::reference>
```
and
```
__is_cpp17_forward_iterator<_ForwardIterator>::value &&
is_constructible<value_type, typename iterator_traits<_ForwardIterator>::reference>
```
Perhaps create helper traits for those, e.g. `IsConstructibleFromInputIterator`/`IsConstuctibleFromForwardIterator`? It should help cut down on the boilerplate, particularly when repeating the `enable_if` in the function definition. If you decide to do this, it's likely best to do in a follow-up.


================
Comment at: libcxx/include/vector:1091
+template <class _InputIterator, __enable_if_t<__is_exactly_cpp17_input_iterator<_InputIterator>::value &&
+                              is_constructible<_Tp, typename iterator_traits<_InputIterator>::reference>::value,
+                          int> >
----------------
Nit: it's unfortunate that now some of the conditions use `_Tp` and others `value_type`, just from the consistency perspective. I presume it's not possible to use `value_type` here because it's not in scope yet at this point. Would it make sense to go the other way round and use `_Tp` in other places instead of `value_type`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131868