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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added inline comments.


================
Comment at: pstl/include/pstl/internal/algorithm_fwd.h:19
 
 _PSTL_HIDE_FROM_ABI_PUSH
 
----------------
I believe you need to add `pstl/include` to
https://github.com/google/llvm-premerge-checks/blob/main/scripts/clang-tidy.ignore
(Although why this file is in a Google repo instead of the llvm-project repo, I don't know!)


================
Comment at: pstl/include/pstl/internal/algorithm_fwd.h:269
 _ForwardIterator
-__pattern_find_if(_ExecutionPolicy&&, _ForwardIterator, _ForwardIterator, _Predicate, _IsVector,
-                  /*is_parallel=*/std::false_type) noexcept;
+__pattern_find_if(_Tag __tag, _ExecutionPolicy&&, _ForwardIterator __first, _ForwardIterator __last,
+                  _Predicate __pred) noexcept;
----------------
Throughout: On function declarations, I think it's unhelpful to have parameter names like `_Tag __tag` or (below) `_ExecutionPolicy&& __exec`. I would remove those parameter names. The names `__first, __last, __pred` are arguably more helpful; but for consistency I'd probably remove them too. (Or rather, don't add them.)


================
Comment at: pstl/include/pstl/internal/execution_impl.h:30-32
+__is_iterator_of(int) -> decltype(
+    std::conjunction<std::is_base_of<
+        _IteratorTag, typename std::iterator_traits<std::decay_t<_IteratorTypes>>::iterator_category>...>{});
----------------
`decltype(T{})` is just `T`.


================
Comment at: pstl/include/pstl/internal/execution_impl.h:38-41
+template <typename... _IteratorTypes>
+struct __is_random_access_iterator : decltype(__is_iterator_of<std::random_access_iterator_tag, _IteratorTypes...>(0))
+{
+};
----------------
Could this be:
```
template <typename... _IteratorTypes>
using __are_random_access_iterators = decltype(__internal::__is_iterator_of<
    std::random_access_iterator_tag, _IteratorTypes...>(0));
```
(Notice the ADL-proofing on `__is_iterator_of`.)

IIUC, the intent is that `__are_random_access_iterators<int*, char*>` is `true_type` but `__are_random_access_iterators<int*, std::list<int>::iterator>` is `false_type`, right?

Or if C++17 is permitted, then
```
template <typename... _IteratorTypes>
using __are_random_access_iterators = std::bool_constant<
    (std::is_base_of_v<std::random_access_iterator_tag,
                       typename std::iterator_traits<_IteratorTypes>::iterator_category> && ...)
>;
```


================
Comment at: pstl/include/pstl/internal/execution_impl.h:120
+
+using __par_backend_tag =
+#ifdef _PSTL_PAR_BACKEND_TBB
----------------
Rather than split the declaration across `#if`s, it would be easier to read (for both humans and computers) if this were just
```
#if defined(_PSTL_PAR_BACKEND_TBB)
 using __par_backend_tag = __tbb_backend;
#elif _PSTAL_PAR_BACKEND_SERIAL
 using __par_backend_tag = __serial_backend;
#else
 #error "A parallel backend must be specified"
#endif
```
(also note the probably-accidental inconsistent use of `defined` versus non-zero, and the bogus semicolon at the end of line 126)


================
Comment at: pstl/include/pstl/internal/execution_impl.h:129-142
+template <class _IsVector>
+struct __serial_tag
+{
+    using __is_vector = _IsVector;
+};
+
+template <class _IsVector>
----------------
IIUC, `__is_vector` is always `true_type` or `false_type`?
Would it be simpler to define...
```
template<bool _IsParallel, bool _IsVector>
struct __bikeshed_tag {
    using __is_vector = std::bool_constant<_IsVector>;
    using __is_parallel = std::bool_constant<_IsParallel>;
    using __backend_tag = std::conditional_t<_IsParallel, __par_backend_tag, void>;
};

template<bool _IsVector>
using __parallel_tag = __bikeshed_tag<true, _IsVector>;

template<bool _IsVector>
using __serial_tag = __bikeshed_tag<false, _IsVector>;
```
And then if you ever found any need to pattern-match on `__bikeshed_tag<X, true>`, it wouldn't be so awkward.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104492