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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante accepted this revision.
Mordante added a comment.
This revision is now accepted and ready to land.

LGTM!



================
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)
----------------
philnik wrote:
> 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.
> 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

Thanks I missed that in the original review.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131868