Skip to content

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

Merged
hjmjohnson merged 1 commit intoInsightSoftwareConsortium:masterfrom
N-Dekker:Let-New-initialize-all-data
Mar 1, 2024
Merged

ENH: Let x::New() initialize the created object by doing new x()#4481
hjmjohnson merged 1 commit intoInsightSoftwareConsortium:masterfrom
N-Dekker:Let-New-initialize-all-data

Conversation

@N-Dekker
Copy link
Copy Markdown
Contributor

@N-Dekker N-Dekker commented Feb 27, 2024

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.

@github-actions github-actions Bot added type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct area:Core Issues affecting the Core module labels Feb 27, 2024
@N-Dekker N-Dekker force-pushed the Let-New-initialize-all-data branch from b7648d7 to 1d77c81 Compare February 27, 2024 18:34
@N-Dekker N-Dekker changed the title Let New() initialize all member data, even when the default-constructor is defaulted ENH: Let x::New() initialize all member data, by doing new x() Feb 27, 2024
@N-Dekker N-Dekker force-pushed the Let-New-initialize-all-data branch from 1d77c81 to 6453b6f Compare February 27, 2024 18:42
@N-Dekker N-Dekker marked this pull request as ready for review February 27, 2024 18:43
@github-actions github-actions Bot added the type:Enhancement Improvement of existing methods or implementation label Feb 27, 2024
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.
@N-Dekker N-Dekker force-pushed the Let-New-initialize-all-data branch from 6453b6f to 45f534a Compare February 27, 2024 19:07
@N-Dekker N-Dekker changed the title ENH: Let x::New() initialize all member data, by doing new x() ENH: Let x::New() initialize the created object by doing new x() Feb 27, 2024
@issakomi
Copy link
Copy Markdown
Member

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
Copy link
Copy Markdown
Contributor Author

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):

class DerivedClass : public itk::LightObject
{
public:
ITK_DISALLOW_COPY_AND_MOVE(DerivedClass);
using Self = DerivedClass;
using Pointer = itk::SmartPointer<Self>;
itkSimpleNewMacro(DerivedClass);
itkGetConstMacro(Data, int);
protected:
// Defaulted default-constructor, essential to this test.
DerivedClass() = default;
// Defaulted destructor.
~DerivedClass() override = default;
private:
int m_Data; // Without {} initializer, for the purpose of this test.
};

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

@issakomi
Copy link
Copy Markdown
Member

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
Copy link
Copy Markdown
Contributor Author

N-Dekker commented Feb 28, 2024

@blowekamp I think this pull request may also be of interest to you 😃, as you had concerns about uninitialized data, as you mentioned at #4473 (comment)

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

@blowekamp
Copy link
Copy Markdown
Member

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

@N-Dekker
Copy link
Copy Markdown
Contributor Author

N-Dekker commented Feb 28, 2024

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
Copy link
Copy Markdown
Member

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
Copy link
Copy Markdown
Contributor Author

N-Dekker commented Feb 28, 2024

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.

@hjmjohnson hjmjohnson merged commit 1986b54 into InsightSoftwareConsortium:master Mar 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:Core Issues affecting the Core module type:Enhancement Improvement of existing methods or implementation type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants