InsightSoftwareConsortium / ITK

Insight Toolkit (ITK) -- Official Repository. ITK builds on a proven, spatially-oriented architecture for processing, segmentation, and registration of scientific images in two, three, or more dimensions.
https://itk.org
Apache License 2.0
1.4k stars 662 forks source link

Add and use `itk::ImageBase::AllocateInitialized()` #4479

Closed N-Dekker closed 6 months ago

N-Dekker commented 6 months ago

The existing three possible ways to call Allocate(bool = false) (either by Allocate(true), Allocate(false), or Allocate()) may accidentally be confused, especially by novice ITK users.

The proposed AllocateZeroInitializedPixelBuffer() is intended to reduce the chance of such a confusion, and improve code readability. It is equivalent to Allocate(true).


For the record, "AllocateZeroInitializedPixelBuffer" was renamed to "AllocateInitialized", as suggested at https://github.com/InsightSoftwareConsortium/ITK/pull/4479#issuecomment-1964624698

N-Dekker commented 6 months ago

/azp run

blowekamp commented 6 months ago

I find the member function name over verbose.

dzenanz commented 6 months ago

AllocateInitialized for the name? Docstring should have sufficient explanation about default values.

N-Dekker commented 6 months ago

AllocateInitialized for the name?

Thanks for the suggestion @dzenanz. However, I would like the name to be self-explanatory. Until now it was a common practice to add a comment to actually explain the meaning of Allocate(true) with each Allocate(true) call. For example:

m_TemporaryPointer->Allocate(true); // initialize buffer to zero

https://github.com/InsightSoftwareConsortium/ITK/blob/adcc46bf9600b78bda422c49a8aca4611c1751d1/Modules/Core/Common/include/itkFloodFilledFunctionConditionalConstIterator.hxx#L86

So do you (both) think the name AllocateInitialized is sufficiently self-explanatory? So that most people won't feel the need to look up the meaning, or add a comment like "initialize buffer to zero"?

blowekamp commented 6 months ago

So do you (both) think the name AllocateInitialized is sufficiently self-explanatory? So that most people won't feel the need to look up the meaning, or add a comment like "initialize buffer to zero"?

I think the name is sufficient 👍