This is an archive of the discontinued LLVM Phabricator instance.

Paths

Table of Contentst

-
pstl/include/pstl/internal/
-
include/
-
pstl/
-
internal/
-
utils.h

Differential D114624

[pstl] Fix incorrect usage of std::invoke_result
ClosedPublic

Authored by rarutyun on Nov 26 2021, 3:08 AM.

Download Raw Diff

Details

Reviewers

ldionne
MikeDvorskiy
• Quuxplusone

Group Reviewers

Restricted Project

Commits

rGf824bb0e36fc: [pstl] Fix incorrect usage of std::invoke_result

Summary

std::invoke_result takes function object type and arguments separately (unlike std::result_of) so, std::invoke_result_t<F()> usage is incorrect.

On the other hand, we don't need std::invoke() semantics here at all. So just simplifying the code without extra dependency and use trailing return type as the fix.

Diff Detail

Repository: rG LLVM Github Monorepo

Event Timeline

rarutyun requested review of this revision.Nov 26 2021, 3:08 AM

rarutyun created this revision.

Herald added a reviewer: Restricted Project. · View Herald TranscriptNov 26 2021, 3:08 AM

Herald added a subscriber: libcxx-commits. · View Herald Transcript

rarutyun mentioned this in D104492: [libc++][pstl] Implement tag dispatching.Nov 26 2021, 3:09 AM

rarutyun added a child revision: D104492: [libc++][pstl] Implement tag dispatching.Nov 26 2021, 3:12 AM

LGTM

Harbormaster completed remote builds in B136185: Diff 389949.Nov 26 2021, 3:30 AM

This revision was not accepted when it landed; it landed in state Needs Review.Nov 26 2021, 6:32 AM

Closed by commit rGf824bb0e36fc: [pstl] Fix incorrect usage of std::invoke_result (authored by rarutyun). · Explain Why

This revision was automatically updated to reflect the committed changes.

rarutyun added a commit: rGf824bb0e36fc: [pstl] Fix incorrect usage of std::invoke_result.

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?

In 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.

The PSTL is NOT being tested in the CI. I was hoping that someone could pick up D103198, which adds it to the CI.

Revision Contents

Path

Size

pstl/

include/

pstl/

internal/

utils.h

8 lines

Diff 390040

pstl/include/pstl/internal/utils.h

	Show First 20 Lines • Show All 57 Lines • ▼ Show 20 Lines
	}			}

	template <typename _Fp>			template <typename _Fp>
	void __invoke_if_not(std::true_type, _Fp)			void __invoke_if_not(std::true_type, _Fp)
	{			{
	}			}

	template <typename _F1, typename _F2>			template <typename _F1, typename _F2>
	typename std::invoke_result<_F1()>::type			auto
	__invoke_if_else(std::true_type, _F1 __f1, _F2)			__invoke_if_else(std::true_type, _F1 __f1, _F2) -> decltype(__f1())
	{			{
	return __f1();			return __f1();
	}			}

	template <typename _F1, typename _F2>			template <typename _F1, typename _F2>
	typename std::invoke_result<_F2()>::type			auto
	__invoke_if_else(std::false_type, _F1, _F2 __f2)			__invoke_if_else(std::false_type, _F1, _F2 __f2) -> decltype(__f2())
	{			{
	return __f2();			return __f2();
	}			}

	//! Unary operator that returns reference to its argument.			//! Unary operator that returns reference to its argument.
	struct __no_op			struct __no_op
	{			{
	template <typename _Tp>			template <typename _Tp>
	▲ Show 20 Lines • Show All 95 Lines • Show Last 20 Lines