Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

STYLE: Remove this->MakeOutput(0) calls from constructors in Core #4654

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

STYLE: Remove this->MakeOutput(0) calls from constructors in Core #4654

wants to merge 1 commit into from
+9 −13

Conversation

N-Dekker
Copy link
Contributor

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

Intended for after the release of ITK v5.4.0

@github-actions github-actions bot added area:Core Issues affecting the Core module type:Style Style changes: no logic impact (indentation, comments, naming) labels May 10, 2024
@hjmjohnson hjmjohnson added this to the ITK 5.4.1 milestone May 10, 2024
N-Dekker added a commit to SuperElastix/elastix that referenced this pull request May 10, 2024
@N-Dekker
STYLE: Remove this->MakeOutput(0) calls from constructors
637cf02 
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
blowekamp
blowekamp reviewed May 10, 2024
View reviewed changes
Modules/Core/Common/include/itkImageSource.hxx Outdated
this->ProcessObject::SetNumberOfRequiredOutputs(1);
this->ProcessObject::SetNthOutput(0, output.GetPointer());
this->ProcessObject::SetNthOutput(0, TOutputImage::New().GetPointer());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@blowekamp blowekamp May 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So that was a "no" to the request for documentation?

Copy link
Contributor Author

@N-Dekker N-Dekker May 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean "As the idea would be to use New(), rather that MakeOutput, in multiple places in the source tree"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@N-Dekker N-Dekker mentioned this pull request May 10, 2024
Merged
N-Dekker added a commit to SuperElastix/elastix that referenced this pull request May 10, 2024
@N-Dekker
STYLE: Remove this->MakeOutput(0) calls from constructors
a5b7960 
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
@N-Dekker N-Dekker marked this pull request as ready for review May 21, 2024 08:12
@N-Dekker
Copy link
Contributor Author

I think this pull request may also be merged now, as v5.4.0 has been tagged (#4603 (comment))

@N-Dekker N-Dekker mentioned this pull request May 21, 2024
Closed
@N-Dekker
STYLE: Remove this->MakeOutput(0) calls from constructors in Core
9120566 
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 N-Dekker force-pushed the Remove-MakeOutput-from-Core-constructors branch from 6124014 to 9120566 Compare May 21, 2024 15:18
@N-Dekker N-Dekker marked this pull request as draft June 22, 2024 11:42
@N-Dekker
Copy link
Contributor Author

@blowekamp
Copy link
Member

@N-Dekker Can this PR be closed, with #4688 addressing it?

@N-Dekker
Copy link
Contributor Author

@N-Dekker Can this PR be closed, with #4688 addressing it?

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 🤷

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@blowekamp blowekamp blowekamp left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
area:Core Issues affecting the Core module type:Style Style changes: no logic impact (indentation, comments, naming)
Projects
None yet
Milestone
ITK 5.4.1
Development

Successfully merging this pull request may close these issues.
3 participants
@N-Dekker @blowekamp @hjmjohnson