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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added inline comments.


================
Comment at: libcxx/include/vector:401
+                          int> = 0>
+  _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_AFTER_CXX17 vector(_InputIterator __first, _InputIterator __last);
+
----------------
var-const wrote:
> 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.
See D132016.


================
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)
----------------
var-const wrote:
> 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.
I'm planning to do that later.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131868