BUG: Rework API for DataMap and DeepCopy in DataObject. #864
Conversation
imikejackson
added
bug
imikejackson
requested review from
mmarineBlueQuartz,
JDuffeyBQ and
nyoungbq
imikejackson
force-pushed
the
topic/deep_copy_failures
branch
from
1574138
to
87026c2
Compare
imikejackson
force-pushed
the
topic/deep_copy_failures
branch
from
87026c2
to
6656d8c
Compare
imikejackson
force-pushed
the
topic/deep_copy_failures
branch
2 times, most recently
from
5f6a520
to
943f344
Compare
imikejackson
force-pushed
the
develop
branch
2 times, most recently
from
67a7875
to
12c9140
Compare
imikejackson
force-pushed
the
topic/deep_copy_failures
branch
from
943f344
to
9d8546b
Compare
|
@JDuffeyBQ @mmarineBlueQuartz @nyoungbq Did we want to actually do something more with this as in changing the API? Otherwise I'll rename the PR to something like "Updating error message" |
imikejackson
force-pushed
the
topic/deep_copy_failures
branch
from
09d289b
to
c9c2cf3
Compare
|
@imikejackson I'm gonna be honest, it's been a while, so I don't really remember exactly what API this PR was aiming to fix in the first place. However, if the point is percolating errors up the call tree as the description suggests then I think it's a good idea to go further with this, since it will be easier to debug and more stable. |
|
I think there was some early discussion on slack about this. But, yes, I think the issue is that the error should be percolated up the call chain instead of just crashing at runtime. Either that or every call that uses this API should be put inside of a try-catch in order to not crash. |
imikejackson
force-pushed
the
topic/deep_copy_failures
branch
3 times, most recently
from
1425cc2
to
e15f583
Compare
imikejackson
force-pushed
the
topic/deep_copy_failures
branch
from
e15f583
to
9fbac2e
Compare
We need to be able to report errors back up the call tree if possible. Signed-off-by: Michael Jackson <mike.jackson@bluequartz.net>
Co-authored-by: Nathan Young <nathan.young@bluequartz.net>
Signed-off-by: Michael Jackson <mike.jackson@bluequartz.net>
imikejackson
force-pushed
the
topic/deep_copy_failures
branch
from
e2f414f
to
c2b4b00
Compare
| } | ||
| if(m_DataMap.insert(ptr)) | ||
| { | ||
| ptr->addParent(this); | ||
| return true; | ||
| return {true}; |
There was a problem hiding this comment.
| return {true}; | |
| return {{true}}; |
Since we use std::expected (from extraneous nonstd library) I believe we need the second bracket to construct it inside the Result object to pass it to the default constructor. This ensures that the valid() and invalid() Result wrapper methods function correctly. @JDuffeyBQ should probably verify I am correct in this behavior since I'm shaky on how the result is intended to function.
There was a problem hiding this comment.
I think that issue was with a particular version of gcc. Testing on my Windows machine both ways compile and produce the same result.
Also in this particular case Result<bool> is redundant since the result already conveys the success or failure so it should just be Result<void>.
There was a problem hiding this comment.
There is a lot of places that this is going to hit. I'm slowly going down this rabbit hole a bit at a time.
@JDuffeyBQ Ironically I started off with a Result<> but then changed to Result thinking that is what was needed. But you are correct in the fact that you return a "MakeErrorResult" will set the result to invalid, so that works.
| { | ||
| return MakeErrorResult<bool>(-1674, fmt::format("AttributeMatrix: CanInsert() Failed with object {}. totalTuples={} incomingTupleCount={}", obj->getName(), totalTuples, incomingTupleCount)); | ||
| } | ||
| return {true}; |
There was a problem hiding this comment.
| return {true}; | |
| return {{true}}; |
Same justification
| } | ||
| return true; | ||
| return {true}; |
There was a problem hiding this comment.
| return {true}; | |
| return {{true}}; |
Same justification
We need to be able to report errors back up the call tree if possible.