[libcxx-commits] [PATCH] D114624: [pstl] Fix incorrect usage of std::invoke_result

[Ruslan Arutyunyan via Phabricator via libcxx-commits]

rarutyun added a comment.

In D114624#3155858 <https://reviews.llvm.org/D114624#3155858>, @Quuxplusone wrote:

> Obviously correct, :shipit:. But some non-blocking nits:
> (1) If we have any tests related to PSTL, then surely it's trivial to add a regression test for this. (But I'm not sure we do.)
> (2) The indentation would read better as
>
>   template <typename _F1, typename _F2>
>   auto __invoke_if_else(std::false_type, _F1, _F2 __f2)
>       -> decltype(__f2())
>   {
>
> (3) It's mildly weird that `__invoke_if` and `__invoke_if_not` return `void` on both branches, but `__invoke_if_else` returns `auto` on both branches. You sure you couldn't just make the return type `void`?

(1) Yes. We do have tests. I am not sure why CI didn't catch that. Locally (when developing other activity) I caught it relatively quickly. It was a mistake that when switching from `std::result_of` to `std::invoke_result`. At the same time, neither is necessary.
Since it's implementations details, I doubt we should create tests directly to that API (if I understood you correctly). They are already tested via algorithms (not all) call.

(2). It's applied clang-format. I remember your point that "it's not boss here" but I believe this sub-project lives with clang-format rules always applied. And in oneDPL we use this PSTL project as upstream and the same clang-format rules. It's just easier to look at diff and make code base synchronization.

(3). I also thought about the same today when was doing this patch. I think the idea for `__invoke_if_else` is you may write something like `auto x = __invoke_if_else(some-args)` even with different return type at compile-time, while for the functions like `__invoke_if` we obviously cannot do the similar thing. Current code base relies on returning something but `void` from `__invoke_if_else`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114624