Named lambda operation #1667
Named lambda operation #1667
Conversation
ginkgo-bot
added
reg:testing
core/test/base/executor.cpp
Outdated
|
|
||
| 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)
core/test/base/executor.cpp
Outdated
|
|
||
| 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)
| @@ -659,6 +659,17 @@ class Executor : public log::EnableLogging<Executor> { | |||
| this->run(op); | |||
| } | |||
|
|
|||
| 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.
MarcelKoch
force-pushed
the
named-lambda-operation
branch
from
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>
MarcelKoch
force-pushed
the
named-lambda-operation
branch
from
0b88ed4 to
ac8bbdd
Compare
MarcelKoch
added
1:ST:ready-to-merge
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.