Use FBContainer for all Function Blocks #198
Conversation
azoitl
force-pushed
the
fbContainerRework
branch
from
be2b1db
to
f06ec75
Compare
azoitl
force-pushed
the
fbContainerRework
branch
from
f06ec75
to
141f1ae
Compare
| @@ -564,6 +541,9 @@ CFunctionBlock *CFunctionBlock::getFB(forte::core::TNameIdentifier::CIterator &p | |||
| //only check for adpaters if it we have the last entry in the line | |||
| retVal = getAdapter(*paNameListIt); | |||
There was a problem hiding this comment.
I found an invalid nullptr access via getAdapter caused by an unititialized interface spec in EMB_RES, which manifests now because this is overriding CFBContainer::getFB and thus running this code in more situations. I have opened a PR azoitl#3 to address the problem, which also fixed the tests for me.
There was a problem hiding this comment.
This is an interesting issue. Because I tested in addition to the unit tests locally by deploying an app to 4diac FORTE.
Should we do some additional protection against nullptrs for interfacespecs? Should we turn it into an ref? Maybe in a separate PR.
azoitl
force-pushed
the
fbContainerRework
branch
from
71d65fb
to
7df4714
Compare
src/core/fbcontainer.cpp
Outdated
| EMGMResponse CFBContainer::addDirectlyInstantiatedFB(CFunctionBlock* paFuncBlock) { | ||
| EMGMResponse retVal = EMGMResponse::InvalidObject; | ||
| if(paFuncBlock != nullptr) { | ||
| if(paFuncBlock->initialize()) { |
There was a problem hiding this comment.
Should we not better do this in a (generated) initialize() of the outer FB?
There was a problem hiding this comment.
I thought it would be better to keep it at some central place.
There was a problem hiding this comment.
The current implementation in eclipse-4diac/4diac-ide#332 generates these calls into the constructor without checking the return value. Maybe we could call this from a generated initialize() instead and also check for the return value there?
| } | ||
|
|
||
| CFBContainer::~CFBContainer() { | ||
| for (TFunctionBlockList::iterator itRunner(mFunctionBlocks.begin()); itRunner != mFunctionBlocks.end(); ++itRunner) { | ||
| CTypeLib::deleteFB(*itRunner); |
There was a problem hiding this comment.
Will this not attempt to delete internal FBs added from variables as well?
There was a problem hiding this comment.
You are totally right. I added code in CBasicFB to handle that case.
src/core/fbcontainer.cpp
Outdated
| EMGMResponse CFBContainer::addDirectlyInstantiatedFB(CFunctionBlock* paFuncBlock) { | ||
| EMGMResponse retVal = EMGMResponse::InvalidObject; | ||
| if(paFuncBlock != nullptr) { | ||
| if(paFuncBlock->initialize()) { |
There was a problem hiding this comment.
The current implementation in eclipse-4diac/4diac-ide#332 generates these calls into the constructor without checking the return value. Maybe we could call this from a generated initialize() instead and also check for the return value there?
As basic and simple fbs can have FBs as children and to reduce a lot of code complexity in the FBContainer all FBs are now FBContainers. Also-by: Martin Jobst <[email protected]>
azoitl
force-pushed
the
fbContainerRework
branch
from
7df4714
to
6fc697e
Compare
You are right we should not do that as I originally proposed. To reduce code duplication and give us more flexibility for the future I had the idea of doing the internal FB initialization in |
Good idea. How about we take this one step further:
The idea would be that everything added in the constructor is assumed to be pre-allocated and thus un- |
|
@mx990 I'm half half on this idea. On the one hand it would harmonize some code. In the other hand larger cfbs with cfbs as children would result in rather big allocations. Not sure if it is therefore better to have cfbs different. However the intention of this commit is to get children of Basic and Simple FBs better integrated. I think with this PR this is achieved. Improving the CFB I would see in another PR. |
|
Hi, i get the Following Errors: |
|
ah, i need to re-export this block with latest nightly ? |
|
worked |
|
@azoitl Sir, with this commit I get the following errors: With both latest milestone or nightly IDE. |
eclipse-4diac#198 broke the interface used by the trace CTF implementation
eclipse-4diac#198 broke the interface used by the trace CTF implementation
eclipse-4diac#198 broke the interface used by the trace CTF implementation
#198 broke the interface used by the trace CTF implementation
@kumajaya this is strange. Can you provide more information what you are doing. Because for me deployment worked as it should. However I was away the last two weeks maybe be now it is already fixed. |
It's an empty EMB_RES but as ernstblechaPT mentioned, the issue has been fixed. Beside that, I also found incorrect request ID in bootfile which could probably be fixed by eclipse-4diac/4diac-ide#361. |
I still don't get why this issue has not shown up in my tests. Did it happen on the first or on a consecutive deployment? |
It happened on consecutive deployments with a lot "INVALID_STATE" errors, then I started with a new empty project with the same result. I found reverting this PR fixed the problem temporary until #204 available. |
@kumajaya thx for the confirmation. This explains what I missed to test and why the #203 and #204 fix the issue. |
As basic and simple fbs can have FBs as children and to reduce a lot of code complexity in the FBContainer all FBs are now FBContainers.