Named lambda operation#1667
Conversation
pratikvn
left a comment
There was a problem hiding this comment.
Minor typo. Otherwise looks good!
|
|
||
| exec->run([] {}, [] {}, [] {}, [] {}); | ||
|
|
||
| ASSERT_EQ("unname", name_logger->op_name); |
There was a problem hiding this comment.
| ASSERT_EQ("unname", name_logger->op_name); | |
| ASSERT_EQ("unnamed", name_logger->op_name); |
There was a problem hiding this comment.
I think I'm going to delete this test. The 'default' name is not really important, since we don't specify it publicly. Testing that value seems rather pointless.
There was a problem hiding this comment.
I think it makes sense to test that the value returned is a valid string literal and not an invalid pointer (which would otherwise be very unlikely to be found)
|
|
||
| exec->run([] {}, [] {}, [] {}, [] {}); | ||
|
|
||
| ASSERT_EQ("unname", name_logger->op_name); |
There was a problem hiding this comment.
I think it makes sense to test that the value returned is a valid string literal and not an invalid pointer (which would otherwise be very unlikely to be found)
|
|
||
| template <typename ClosureOmp, typename ClosureCuda, typename ClosureHip, | ||
| typename ClosureDpcpp> | ||
| void run(std::string name, const ClosureOmp& op_omp, |
There was a problem hiding this comment.
Adding a new overload, would it make sense to extend this to include ReferenceExecutor? We can't change the other one, since it would break interface
There was a problem hiding this comment.
I think it would make sense, so I will add that.
There was a problem hiding this comment.
Any ordering preference? I would add it before op_omp, since I think we usually order them that way. But of course, this is then not compatible with the other overload.
There was a problem hiding this comment.
Sounds sensible, since this is a new overload. We can't add reference to the other one though, since that would create ambiguities with the std::string parameter and the additional templated parameter.
There was a problem hiding this comment.
I guess we could think about deprecating the other overload. Adding a name to an operation seems quite useful, especially since all ginkgo kernels are already named.
6e3f78d to
0b88ed4
Compare
- test only for existence of default name - add closure for reference op - deprecate lambda run without name Co-authored-by: Pratik Nayak <pratik.nayak@kit.edu> Co-authored-by: Tobias Ribizel <mail@ribizel.de>
0b88ed4 to
ac8bbdd
Compare
This merge adds an overload for `Executor::run`, which requires a name for the lambda operation, and also a reference lambda operation. The other overload has been deprecated. Related PR: ginkgo-project#1667
This PR enables the naming of lambda operations.