Closed diegoferigo closed 5 years ago
Ack for the first commit.
For the second, for what I understand:
core::Signal signalStack(core::Signal::DataFormat::NONCONTIGUOUS, portInfo.dataType);
portInfo.dataType);
auto signal = std::make_shared<core::Signal>(std::move(signalStack));
is completely equivalent to:
core::Signal signalStack(core::Signal::DataFormat::NONCONTIGUOUS, portInfo.dataType);
portInfo.dataType);
auto signal = std::make_shared<core::Signal>(signalStack);
for a stack-allocated POD, there is no ownership to be transferred, so std::make_shared
will allocate no matter what. shared_ptr
needs to point to an object allocated on the heap, and it can't point to an object on the stack (even because the stack object will become invalid as soon as the function returns.
I do not think you can avoid dynamic memory allocation if you return shared_ptr
, unless you use a cache of shared_ptr
and you return those, as in the case of the coder. However I do not think that is a tragic issue, we can change it in the Blockfactory 2. : )
is completely equivalent to:
From the Travis failure, apparently the std::move
is actually doing something.
I hoped that somehow the compiler was smart enough to keep the memory :/ Well, let's keep only the first commit for the time being.
However I do not think that is a tragic issue, we can change it in the Blockfactory 2. : )
I plan to release a v0.8
asap, so we can still slightly change the APIs in the following one or two releases. I think it is almost ready, this PR was the last one I wanted to be included.
From the Travis failure, apparently the std::move is actually doing something.
It is strange because on my machine (bionic) tests do not fail, and I tried with clang7
, gcc7
and gcc8
. I don't know what's going on in Travis. And btw the SimulinkBlockInformation
class does not have any coverage yet.
It is strange because on my machine (bionic) tests do not fail, and I tried with
clang7
,gcc7
andgcc8
. I don't know what's going on in Travis. And btw theSimulinkBlockInformation
class does not have any coverage yet.
Indeed! So the failures are related to the de-pimpl of Signal? Perhaps there is some std::move(signal)
around that is creating (for some reason) problems?
Indeed! So the failures are related to the de-pimpl of Signal?
Yes
Perhaps there is some std::move(signal) around that is creating (for some reason) problems?
The SignalUnitTest.cpp
is quite simple :thinking: What's strange is that the Travis VM in the combination bionic + Release gets stuck and timeouts, something I never experienced. I do not exclude a problem from their side, let's try tomorrow.
Today with fresh eyes I realized that:
const
version. Now it should be fixed.I'm going to remove the second commit, wait CI to succeed, and merge this PR.
We should think about how to refactor core::Signal
in such a way that is easier to maintain, right now the usage of the void*
buffer requires quite a lot of hacky casts. However, during the last refactor I didn't manage to find a smarter solution due to the constraints we have especially coming from the Simulink way to handle and share signal memory.
In view of #27, this PR:
core::Block
core::Signal
objects fromSimulinkBlockInformation::get{In,Out}putPortSignal()
methods~@traversaro The second commit is just a tentative to avoid changing the interface, the solution is not too hacky but definitely not clean. ~It should do its job though.~