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

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const added inline comments.


================
Comment at: libcxx/include/vector:401
+                          int> = 0>
+  _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_AFTER_CXX17 vector(_InputIterator __first, _InputIterator __last);
+
----------------
Mordante wrote:
> I find it a bit odd to add `_LIBCPP_HIDE_FROM_ABI` in an NFC patch, it's also not mentioned in the commit message.
+1 -- this looks like a good change, but it should probably be in a separate patch.


================
Comment at: libcxx/include/vector:1003
+template <class _ForwardIterator, class>
+_LIBCPP_CONSTEXPR_AFTER_CXX17 void
 vector<_Tp, _Allocator>::__construct_at_end(_ForwardIterator __first, _ForwardIterator __last, size_type __n)
----------------
huixie90 wrote:
> Mordante wrote:
> > Here you removed and enable_if, is that really intended?
> > I'm a bit concerned the CI passes with this change.
> Technically the `enable_if` still applies since the default template argument with enable_if already exists in the function declaration few hundred lines above. But I agree it is not obvious since the declaration and definition are few hundred lines apart from each other.
> 
> Also I agree that using non-type template is better because it is consistent with others, although technically it is not required to use non-type template parameter since there is only one overload
Consider also "inlining" the definitions of the affected functions so we can avoid having to deal with SFINAE twice.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131868