[libcxx-commits] [PATCH] D122173: [libc++][ranges] Implement ranges::transform

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added inline comments.


================
Comment at: libcxx/include/__algorithm/ranges_transform.h:44
+
+  template <class _InputIterator, class _Sentinel,
+            class _OutputIterator,
----------------
var-const wrote:
> Mordante wrote:
> > Is there a reason why the naming of the types and variables differs from the other ranges algorithms?
> > For example `ranges_find.h` uses 
> > ```
> >   template <input_iterator _Ip, sentinel_for<_Ip> _Sp, class _Tp, class _Proj = identity>
> >     requires indirect_binary_predicate<ranges::equal_to, projected<_Ip, _Proj>, const _Tp*>
> >   _LIBCPP_HIDE_FROM_ABI constexpr
> >   _Ip operator()(_Ip __first, _Sp __last, const _Tp& __value, _Proj __proj = {}) const {
> >     auto __pred = [&](auto&& __e) { return std::forward<decltype(__e)>(__e) == __value; };
> >     return ranges::__find_if_impl(std::move(__first), std::move(__last), __pred, __proj);
> >   }
> > 
> > ```
> > I think it improves our codebase when we keep these names consistent.
> > I'm also not to fond of `_InputIterator` and `_OutputIterator`, they are used in the non-range algorithms and there they use the LegacyIterators. So I like that the range based algorithms use a different naming scheme,
> FWIW, I'm mildly in favor of using longer names (though I can see the argument for terser names given how verbose the template parameters are in these functions). I think range algorithms (both committed and in flight) aren't currently consistent in this regard.
I'm not against longer names in general, but when I read `_InputIterator` I expect a `LegacyIterator` in the `<algorithm>` header. I also would like consistent naming, which can be achieved in two ways. Use the old names in new code or change the old code to use new names.There's a thread on Discord so I think it's easier to discuss it there.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122173