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

ENH: Let `x::New()` initialize the created object by doing `new x()` #4481

Closed N-Dekker closed 5 months ago

N-Dekker commented 6 months ago

Replace new x with new x(), in the definitions of itkSimpleNewMacro and itkFactorylessNewMacro. This ensures that objects created by x::New() are "value-initialized". In practice, this will zero-initialize data members that might otherwise be left uninitialized.

Added GTests to check that x::New() does indeed properly initialize the created object.

issakomi commented 6 months ago

Just for completeness , if a class has any user-provided ctor, () will not initialize members.

#include <iostream>

class A
{
public:
    A() {}
    int x;
    double y;
};

int main()
{
    A * a = new A();
    std::cout << a->x << ' ' << a->y << std::endl;
    delete a;
    return 0;
}
r@deb2:~$ g++ -g test.cpp 
r@deb2:~$ valgrind --track-origins=yes ./a.out 
...
Conditional jump or move depends on uninitialised value(s)
...
N-Dekker commented 6 months ago

Just for completeness , if a class has any user-provided ctor, () will not initialize members.


#include <iostream>

class A
{
public:
  A() {}
  int x;
  double y;
};

Indeed, thanks @issakomi But of course, your example class A really does not want its members to be initialized at all! 😺

Obviously the new x() notation does properly initialize the data in the example that I included with the unit tests, "DerivedClass", having a defaulted default-constructor (while the original new x would have left the data of DerivedClass uninitialized): https://github.com/InsightSoftwareConsortium/ITK/blob/45f534a8a2fcc29a6b523a6efeb151c0559374a4/Modules/Core/Common/test/itkLightObjectGTest.cxx#L29-L49

So I hope you agree that this pull request is a step in the right direction 😃

issakomi commented 6 months ago

Obviously the new x() notation does properly initialize the data in the example that I included with the unit tests

Ah, yes, I see the comment in the example now // Defaulted default-constructor, essential to this test.

N-Dekker commented 6 months ago

@blowekamp I think this pull request may also be of interest to you 😃, as you had concerns about uninitialized data, as you mentioned at https://github.com/InsightSoftwareConsortium/ITK/pull/4473#issuecomment-1957226758

By the way, pull request #4479 (ImageBase::AllocateInitialized()) was also inspired by your concerns about initialization. So I do consider your comments 😃

blowekamp commented 6 months ago

Why do we use '()' here instead of '{}'?

N-Dekker commented 6 months ago

Why do we use '()' here instead of '{}'?

Good question, thanks @blowekamp It appears that in 99,9 % of the cases () and {} are equivalent. But there are some very rare cases in which () would initialize all data, while {} might leave some of the data uninitialized. (99,9 % is just a wild guess of mine.) I'll try to make that clearer in a moment.

blowekamp commented 6 months ago

Why do we use '()' here instead of '{}'?

Good question, thanks @blowekamp It appears that in 99,9 % of the cases () and {} are equivalent. But there are some very rare cases in which () would initialize all data, while {} might leave some of the data uninitialized. (99,9 % is just a wild guess of mine.) I'll try to make that clearer in a moment.

This supports the other comment I was about to make. I think the C++ initialization rule are rather complicated and there are cases I don't fully understand. I'll defer to you for the correctness of this patch.

N-Dekker commented 6 months ago

Ah, I just double-checked. For a class x that has a base class (for example, itk::LightObject), the expression new x() and the expression new x{} are entirely equivalent. Only when x would be an aggregate type, there might be some subtle difference between () and {}. But in the context of this pull request, x is not an aggregate type. So there is no problem here 😃

So in the context of this PR, the choice between () and {} is just a matter of taste. And in this case, I don't have a strong preference between the two.