[libcxx-commits] [PATCH] D104492: [libc++][pstl] Implement tag dispatching

[Ruslan Arutyunyan via Phabricator via libcxx-commits]

rarutyun added inline comments.


================
Comment at: pstl/include/pstl/internal/algorithm_fwd.h:402
 template <class _ForwardIterator, class _OutputIterator>
-_OutputIterator
-__brick_swap_ranges(_ForwardIterator __first, _ForwardIterator __last, _OutputIterator __result,
-                    /*vector=*/std::false_type) noexcept;
+_OutputIterator __brick_swap_ranges(_ForwardIterator, _ForwardIterator, _OutputIterator,
+                                    /*vector=*/std::false_type) noexcept;
----------------
MikeDvorskiy wrote:
> I guess we have wrong clang format changes here. (As far as I remember our profile of clang-format options keeps return type on the separate line).
It's already inconsistent within the file. I suggest to make separate commit if we believe it's important. Locally I've applied clang-format for diff and that's the output


================
Comment at: pstl/include/pstl/internal/algorithm_fwd.h:406
 template <class _RandomAccessIterator, class _OutputIterator>
-_OutputIterator
-__brick_swap_ranges(_RandomAccessIterator __first, _RandomAccessIterator __last, _OutputIterator __result,
-                    /*vector=*/std::true_type) noexcept;
+_OutputIterator __brick_swap_ranges(_RandomAccessIterator, _RandomAccessIterator, _OutputIterator,
+                                    /*vector=*/std::true_type) noexcept;
----------------
MikeDvorskiy wrote:
> See the comment below.
I guess you mean "above" instead of "below". Anyway, the answer is above:) 


================
Comment at: pstl/include/pstl/internal/algorithm_impl.h:308
 _RandomAccessIterator2
-__pattern_walk2_brick(_ExecutionPolicy&& __exec, _RandomAccessIterator1 __first1, _RandomAccessIterator1 __last1,
-                      _RandomAccessIterator2 __first2, _Brick __brick, /*parallel=*/std::true_type)
+__pattern_walk2_brick(__parallel_tag<_IsVector> __tag, _ExecutionPolicy&& __exec, _RandomAccessIterator1 __first1,
+                      _RandomAccessIterator1 __last1, _RandomAccessIterator2 __first2, _Brick __brick)
----------------
MikeDvorskiy wrote:
> As far as I can see, here and in the other same places  we don't in use parameter name __tag in code, excepting type deduction (via decltype) of type that we already have. So, I would propose to omit this parameter name and define __backend_tag as:
> 
> using __backend_tag = typename __parallel_tag<_IsVector>::__backend_tag;
> 
> It is not showstopper because it is not public API. So, it is up to you.
Actually, in some places we use `__tag` parameter but not everywhere. Although, we could do `using backend_tag` with duplicating the whole type from template parameter I would prefer to keep it as it is because it (from my point of view) makes the code simpler and ``backend_tag`` line is not required to be changed if function parameter type is modified. Everything would be correctly deduced automatically would be correctly deduced automaticallyю


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104492