STYLE: Remove this->MakeOutput(0) calls from constructors in Core#4654
STYLE: Remove this->MakeOutput(0) calls from constructors in Core#4654N-Dekker wants to merge 1 commit intoInsightSoftwareConsortium:mainfrom
this->MakeOutput(0) calls from constructors in Core#4654Conversation
github-actions
bot
added
area:Core
Analogue to ITK pull request InsightSoftwareConsortium/ITK#4654 Following C++ Core Guidelines, February 15, 2024, "Don’t call virtual functions in constructors and destructors", https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rc-ctor-virtual
| typename TOutputImage::Pointer output = static_cast<TOutputImage *>(this->MakeOutput(0).GetPointer()); | ||
| this->ProcessObject::SetNumberOfRequiredOutputs(1); | ||
| this->ProcessObject::SetNthOutput(0, output.GetPointer()); | ||
| this->ProcessObject::SetNthOutput(0, TOutputImage::New().GetPointer()); |
There was a problem hiding this comment.
Please add comment like: "Generally MakeOutput(n) should be used, but because a virtual method should not be called in a constructor the output is directly constructed."
It is important to note how things should be done, and why they are not done that way for this case. There have been bugs before related to not using MakeOutput.
There was a problem hiding this comment.
Thanks for the suggestion @blowekamp
There have been bugs before related to not using MakeOutput.
Can you possibly still recall what such a bug looked like? Just for my understanding!
There was a problem hiding this comment.
Don't recall the specifics... There are cases when a Process object can have it's outputs removed, and then during execution it needs to make them again.
Maybe f0d570e?
There was a problem hiding this comment.
Interesting! In general, I think this->MakeOutput(i) calls are not necessary, inside a constructor of a class, when the MakeOutput(i) override of this class unconditionally just does the same OutputType::New() call for any index value i.
There was a problem hiding this comment.
I agree they are not necessary. I am just suggestion adding documentation as to why the canonical way of creating a new output is not being used e.g documenting the exception case.
There was a problem hiding this comment.
So that was a "no" to the request for documentation?
There was a problem hiding this comment.
Oh, sorry Bradley, what do you want there? Something like
// Basically equivalent to SetNthOutput(0, MakeOutput(0)).
this->ProcessObject::SetNthOutput(0, TOutputImage::New().GetPointer())
?
I hope it does not need to be longer than one line of comment. This pull request is intended to simplify the code. The idea is: Just call TOutput::New() if that does the job!
There was a problem hiding this comment.
Sorry, I see your suggestion at https://github.com/InsightSoftwareConsortium/ITK/pull/4654/files#r1596759648 As the idea would be to use New(), rather that MakeOutput, in multiple places in the source tree, I would rather have those considerations elsewhere. Maybe in the ITKSoftwareGuide?
There was a problem hiding this comment.
What do you mean "As the idea would be to use New(), rather that MakeOutput, in multiple places in the source tree"?
There was a problem hiding this comment.
Well, this pull request already proposes to replace three MakeOutput calls with TOutput::New(). I don't think it's nice to have an extensive multi-line comment copy-and-pasted multiple times.
Analogue to ITK pull request InsightSoftwareConsortium/ITK#4654 Following C++ Core Guidelines, February 15, 2024, "Don’t call virtual functions in constructors and destructors", https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rc-ctor-virtual
|
I think this pull request may also be merged now, as v5.4.0 has been tagged (#4603 (comment)) |
In those cases, `MakeOutput(0)` just did `OutputType::New()` anyway. This commit avoids unnecessary casts and calls to virtual functions. Following C++ Core Guidelines, February 15, 2024, "Don’t call virtual functions in constructors and destructors", https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rc-ctor-virtual
N-Dekker
force-pushed
the
Remove-MakeOutput-from-Core-constructors
branch
from
6124014 to
9120566
Compare
|
|
Good question @blowekamp 😃 This PR specifically addresses cases where there is exactly 1 required output. PR #4688 does not yet address those cases. So it's still open 🤷 |
thewtex
modified the milestones:
ITK 6.0 Beta 1,
Backlog -- Will be considered for future releases
4 participants
In those cases,
MakeOutput(0)just didOutputType::New()anyway. This commit avoids unnecessary casts and calls to virtual functions.Following C++ Core Guidelines, February 15, 2024, "Don’t call virtual functions in constructors and destructors",
https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rc-ctor-virtual
Intended for after the release of ITK v5.4.0