[SYCL][ESIMD] Add test proxy for move constructors #5132
Conversation
Because copy constructor works as the default fallback for every case with move constructor disabled or not provided, there is no way for a caller to differentiate between actual call to move constructor and copy constructor. Same goes for copy and move assignment operators. Therefore a test proxy is the only way left to check the move constructor and the move assignment works as expected. Signed-off-by: Yuriy Kochetkov <[email protected]>
|
@kbobrovs, @v-klochkov Would you mind to take a look? I'm worried about |
|
|
||
| #pragma once | ||
|
|
||
| #ifndef __ESIMD_ENABLE_TEST_PROXY |
There was a problem hiding this comment.
It is not quite clear if the test proxy is going to be used in device code - please clarify, and if yes - how. Otherwise, the above #ifndef can be changed to #if !defined(__ESIMD_ENABLE_TEST_PROXY) || defined (__SYCL_DEVICE_ONLY__)
There was a problem hiding this comment.
It's expected for the proxy class to be available in device code, so that it could be incorporated into the ESIMD API classes. See __ESIMD_DECLARE_TEST_PROXY_ACCESS usage.
I've added a comment to the code, please let me know if there are any other concerns
| namespace esimd { | ||
| namespace detail { | ||
|
|
||
| /// The test_proxy class. |
There was a problem hiding this comment.
Please mention that it is used solely for testing purposes, and there is no extra code generated for usual builds.
There was a problem hiding this comment.
Thanks, done by 7802416. I've tried to avoid repeating the identical statements though.
| m_moved = true; | ||
| return *this; | ||
| } | ||
| bool was_moved() const { return m_moved; } |
There was a problem hiding this comment.
sounds somewhat misleading. m_moved does not indicate that *this was moved. It rather tells that *this was either
- constructed via a move constructor or
- another object was moved into it
was_move_destination()? (+ was_created_with_move_ctor() if we need to distinguish the two)
There was a problem hiding this comment.
Nope, we don't need to distinguish the move constructor and move assignment. So something like was_created_by_move() might work, though I'm not sure it looks good in case of the move assignment.
Not sure we should do it though.
It's UB to access source instance after it has been moved to the destination instance, so in terms of logic we might say that in case of std::move it's just the same instance moving around.
It's even more clear if we took into account the corner cases with the chained calls, for example:
// Chained move
test_proxy a;
test_proxy b(std::move(a));
test_proxy c(std::move(b));
assert(a.was_moved()); // UB, logically it's moved to "c"
assert(c.was_moved()); // valid: that's instance "a" that moved here
// Chained move and copy
test_proxy a;
test_proxy b(std::move(a));
test_proxy c(b);
assert(a.was_moved()); // UB, logically it's moved to "b"
assert(b.was_moved()); // valid: that's instance "a" that moved here
assert(c.was_moved() == false); // valid: that's new instance "c" with data from "a"
Does it address your concerns or is it better to change it to the was_created_by_move()?
There was a problem hiding this comment.
Renamed to the suggested was_move_destination() on the second thought by 0c83d18. Also aligned a class attribute code style with other ESIMD API headers.
| bool was_moved() const { return m_moved; } | ||
| }; | ||
|
|
||
| } // namespace detail |
There was a problem hiding this comment.
let's use separate namespace for the test stuff. esimd::detail::test ?
There was a problem hiding this comment.
Done by 0080b4e, please take a look. I haven't added any subdir though, as I don't see any other cases when we would need the test stuff except this specific case.
Looks like the semicolon could possibly lead to the code breakage
in the following case:
if (expression)
__esimd_move_test_proxy(other);
else
statement;
Signed-off-by: Kochetkov, Yuriy <[email protected]>
Signed-off-by: Kochetkov, Yuriy <[email protected]>
Signed-off-by: Kochetkov, Yuriy <[email protected]>
Also the class attribute code style was aligned with other ESIMD API headers Signed-off-by: Kochetkov, Yuriy <[email protected]>
|
@kbobrovs Thanks for review. Please take a closer look onto the
I couldn't avoid such restriction. Is it OK? Is it clear from the comments? |
|
/verify |
To resolve merge conflicts
Because copy constructor works as the default fallback for every case with move
constructor disabled or not provided, there is no way for a caller to
differentiate between actual call to move constructor and copy constructor.
Same goes for copy and move assignment operators.
Therefore a test proxy is the only way left to check the move constructor and
the move assignment works as expected.
The drawback of this patch is a requirement to have
__esimd_move_test_proxyexplicitly stated for every user-defined move constructor and move operator.
We might use default value for any constructor, the flag will be triggered as
needed by any defaulted/implicit move constructor/operator, but user-defined
move constructors and operators still need some explicit trigger provided by
developer.
Signed-off-by: Yuriy Kochetkov [email protected]