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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik marked 2 inline comments as done.
philnik added inline comments.


================
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>
----------------
var-const wrote:
> 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.
I agree that it should be done in a follow-up. Though I'm not sure it's worth it once the function definitions are inlined.


================
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> >
----------------
var-const wrote:
> 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`?
This problem should also not exist anymore once the definitions are inlined.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131868