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.

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

itkImportImageContainer AllocateElements documentation doesn't seem right #4359

Closed
issakomi opened this issue Dec 2, 2023 · 1 comment · Fixed by #4360
Closed

itkImportImageContainer AllocateElements documentation doesn't seem right #4359

issakomi opened this issue Dec 2, 2023 · 1 comment · Fixed by #4360
Labels
type:Documentation Documentation improvement or change

Comments

@issakomi
Copy link
Member

issakomi commented Dec 2, 2023

This ( in itkImportImageContainer.h ) doesn't seem right to me

  /**
   * Allocates elements of the array.  If UseDefaultConstructor is true, then
   * the default constructor is used to initialize each element.  POD date types
   * initialize to zero.
   */
  virtual TElement *
  AllocateElements(ElementIdentifier size, bool UseDefaultConstructor = false) const;

UseDefaultConstructor controls this:

    if (UseDefaultConstructor)
    {
      data = new TElement[size](); // POD types initialized to 0, others use default constructor.
    }
    else
    {
      data = new TElement[size]; // Faster but uninitialized
    }

C++03 value initialization with () is not about using a default constructor or not. A user-provided default constructor will be called in both cases, of course.

The difference is for POD types (notation POD "plain old data" is, BTW, deprecated together with std::is_pod, actually it is std::is_trivial<T>() && std::is_standard_layout<T>() ). For such types, e.g. scalars, () will have effect of zero-initialization, the same as with C++11 {}.

#include <iostream>

class A
{
public:
	A()
	{
		x = 0;
		std::cout << "A() ctor" << std::endl;
	}
	int x;
};

int main()
{
	A * a = new A[2];
	A * b = new A[2]();
	delete [] a;
	delete [] b;
	return 0;
}
r@deb2:~$ ./a.out 
A() ctor
A() ctor
A() ctor
A() ctor

It is e.g. related to RGB images

  using RGBPixelType = itk::RGBPixel<unsigned char>;
  using ImageType = itk::Image<RGBPixelType, 2>;

itk::RGBPixel has a constructor RGBPixel() { this->Fill(0); } and
image->Allocate();
image->Allocate(true);
image->Allocate(false);
will make no difference, the default constructor will be always used (tested).

Again, for e.g. scalar types there is the effect of zero-initialization
int * x = new int[100]();

Any class with a use-provided default constructor, even the simplest one, is no longer trivial

#include <iostream>
#include <type_traits>

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

class B
{
public:
  int x;
};

class C
{
public:
  C() = default;
  int x;
};

int main()
{
  std::cout
    << std::is_standard_layout<A>() << " " << std::is_trivial<A>() << '\n'
    << std::is_standard_layout<B>() << " " << std::is_trivial<B>() << '\n'
    << std::is_standard_layout<C>() << " " << std::is_trivial<C>() << std::endl;
  return 0;
}
r@deb2:~$ ./a.out 
1 0
1 1
1 1

I would suggest to make STYLE changes, remove the part about the "default constructor" from the comment and rename the variable UseDefaultConstructor to UseValueInitialization.

CC @N-Dekker

Edit:

For completeness, in the above example A * a = new A[2]() or A * a = new A[2]{} does not initialize x, no difference with A * a = new A[2], but for B and C (trivial types) B * b = new B[2]() or B * b = new B[2]{} does initialize x to 0. Tested with Valgrind.

@issakomi issakomi added the type:Documentation Documentation improvement or change label Dec 2, 2023
issakomi added a commit to issakomi/ITK that referenced this issue Dec 3, 2023
Comment are variable names better describe the behavior of the
ImportImageContainer.
Closes InsightSoftwareConsortium#4359
issakomi added a commit to issakomi/ITK that referenced this issue Dec 3, 2023
Comments are variable name describe the behavior of the
ImportImageContainer better.
Closes InsightSoftwareConsortium#4359
issakomi added a commit to issakomi/ITK that referenced this issue Dec 3, 2023
Comments are variable name describe the behavior of the
ImportImageContainer better.
Closes InsightSoftwareConsortium#4359
issakomi added a commit to issakomi/ITK that referenced this issue Dec 4, 2023
Comments and the variable name better describe the behavior of the
ImportImageContainer. The variable name in VectorImage has also been updated.
Closes InsightSoftwareConsortium#4359
@N-Dekker
Copy link
Contributor

N-Dekker commented Dec 4, 2023

Thanks @issakomi

FYI, value-initialization is also of interest for an aggregate of trivial and non-trivial components:

struct MyAggregate
{
    std::string s;
    int i;
};

int main()
{
    auto defaultInitialized = new MyAggregate[2];
    auto valueInitialized = new MyAggregate[2]();

    return valueInitialized[0].i;
}

In this case, value-initialization takes care of zero-initializing the integer MyAggregate::i, whereas default-initialization does not necessarily do so. https://godbolt.org/z/xGcWTv7Tc

No need to adjust the text of the issue though, it's fine 👍

issakomi added a commit to issakomi/ITK that referenced this issue Dec 5, 2023
Updated comments and the parameter 'UseValueInitialization' better
describe the behavior of the ImportImageContainer.
The same parameter in VectorImage has also been updated.
Also removed comment from AllocateElements about 'new' and throwing
or not throwing an exception if allocation fails.
Closes InsightSoftwareConsortium#4359
N-Dekker pushed a commit that referenced this issue Dec 5, 2023
Updated comments and the parameter 'UseValueInitialization' better
describe the behavior of the ImportImageContainer.
The same parameter in VectorImage has also been updated.
Also removed comment from AllocateElements about 'new' and throwing
or not throwing an exception if allocation fails.
Closes #4359
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:Documentation Documentation improvement or change
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants