IOTask: AbstractParameter Slice Safety#410
Conversation
ax3l
force-pushed
the
topic-modernizeTaskMakeShared
branch
5 times, most recently
from
58832c3 to
2b6123f
Compare
| Parameter< op > const& p) | ||
| : writable{w}, | ||
| operation{op}, | ||
| parameter{new Parameter< op >(p)} |
There was a problem hiding this comment.
that abstract base class should not be copied to each other. That is object slicing:
https://stackoverflow.com/questions/274626/what-is-object-slicing
https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rc-copy-virtual
https://en.wikipedia.org/wiki/Object_slicing
There was a problem hiding this comment.
Hm, but since we explicitly new on the Parameter< op > this should not be slicing...
At least clang-tidy-6.0 does not find it as such:
clang-tidy-6.0 test.cpp -checks='*,cppcoreguidelines-slicing' --Could still be an AppleClang compiler bug that optimizes it to one.
There was a problem hiding this comment.
Nah, it is likely still slicing since we push IOTasks into std::qeues ... but they are only copies of shared_ptrs of AbstractParameter, so no copy.
Maybe it's just a compiler bug with the implicit/explicit copy constructors. Now it's safe.
ax3l
force-pushed
the
topic-modernizeTaskMakeShared
branch
from
2b6123f to
a981d4a
Compare
include/openPMD/IO/IOTask.hpp
Outdated
| @@ -235,22 +440,22 @@ class IOTask | |||
| */ | |||
| template< Operation op > | |||
| IOTask(Writable* w, | |||
There was a problem hiding this comment.
maybe also make explicitto avoid unintentional implicit conversions
include/openPMD/IO/IOTask.hpp
Outdated
| { } | ||
|
|
||
| template< Operation op > | ||
| IOTask(Attributable* a, |
There was a problem hiding this comment.
maybe also make explicitto avoid unintentional implicit conversions
ax3l
force-pushed
the
topic-modernizeTaskMakeShared
branch
from
a981d4a to
04fcf11
Compare
ax3l
changed the title
ax3l
force-pushed
the
topic-modernizeTaskMakeShared
branch
from
04fcf11 to
0aadc0d
Compare
The IOTask constructor caused problems with Clang 4.0.1 on OSX, where the `Parameter< Operation::CREATE_FILE >` does seem not to copy the `name` member. Thanks to Ray Donnelly for first seeing this! ~~It turns out that we copy the `AbstractParameter` base class, which leads to object slicing.~~ Probably implicit copy constructors are weirdly handled in this old Clang version, so we make them explicitly deleted as the core guidelines recommend. Also, `IOTasks` might better have an explicit copy constructor since we copy it around (`std::queue::push()`, etc.) https://stackoverflow.com/questions/274626/what-is-object-slicing https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rc-copy-virtual https://en.wikipedia.org/wiki/Object_slicing
ax3l
force-pushed
the
topic-modernizeTaskMakeShared
branch
from
0aadc0d to
31a91f7
Compare
1 participant
The
IOTaskconstructor caused problems with Clang 4.0.1 on OSX, where theParameter< Operation::CREATE_FILE >does seem not to copy thenamemember. Thanks to @mingwandroid for first seeing this and debugging a lot with me!It turns out that we copy theAbstractParameterbase class, which leads to object slicing.Probably implicit copy constructors are weirdly handled in this old Clang version, so we make them explicitly deleted as the core guidelines recommend.
Also,
IOTasksmight better have an explicit copy constructor since we copy it around (std::queue::push(), etc.)https://stackoverflow.com/questions/274626/what-is-object-slicing
https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rc-copy-virtual
https://en.wikipedia.org/wiki/Object_slicing