Skip to content

Conversation

@N-Dekker
Copy link
Contributor

VariableLengthVector::AllocateElements just converted any possible exception from new TValue[size] to an ITK exception:

VariableLengthVector<TValue>::AllocateElements(ElementIdentifier size) const
{
try
{
return new TValue[size];
}
catch (...)
{
// Intercept std::bad_alloc and any exception thrown from TValue
// default constructor.
itkGenericExceptionMacro("Failed to allocate memory of length " << size << " for VariableLengthVector.");
}
}

It appears unnecessary for VariableLengthVector to do so. In many other cases, ITK does allow non-ITK exceptions to occur, so end-users have to deal with them anyway.

This pull request was triggered by a comment by Bradley (@blowekamp) at #5693 (comment) saying:

The utility of AllocateElements wrapping a new to throw an exception which requires additional memory allocation to perform ostringstream operations is certainly IMHO, questionable.

@N-Dekker N-Dekker requested a review from blowekamp December 23, 2025 11:09
@github-actions github-actions bot added the area:Core Issues affecting the Core module label Dec 23, 2025
Comment on lines 31 to 33
: m_Data(nullptr)
: m_Data(new TValue[length])
{
Reserve(length);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Simplified the VariableLengthVector(length) constructor by removing its internal Reserve(length) call.

It appears unnecessary for VariableLengthVector to replace exceptions from
`new T[n]` with ITK exceptions.

This commit also simplified the `VariableLengthVector(length)` constructor.
Although AllocateElements is a public member function of VariableLengthVector,
it was only used internally, by VariableLengthVector itself.
@N-Dekker N-Dekker force-pushed the Remove-AllocateElements-from-VariableLengthVector branch from a222c4d to f080d4f Compare December 23, 2025 12:28
Copy link
Member

@blowekamp blowekamp left a comment

Choose a reason for hiding this comment

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

These changes look good to me.

Copy link
Member

@dzenanz dzenanz left a comment

Choose a reason for hiding this comment

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

Looks good on a glance.

@N-Dekker N-Dekker marked this pull request as ready for review December 23, 2025 15:07
Copy link
Member

@thewtex thewtex left a comment

Choose a reason for hiding this comment

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

Nice!

@thewtex thewtex merged commit 86a2bbf into InsightSoftwareConsortium:main Dec 23, 2025
17 checks passed
@issakomi
Copy link
Member

Valgrind seems to be complaining

https://open.cdash.org/builds/10917349/dynamic_analysis

@N-Dekker
Copy link
Contributor Author

Valgrind seems to be complaining

https://open.cdash.org/builds/10917349/dynamic_analysis

Thanks @issakomi, but as far as I can see, those failures are unrelated to this PR (Remove AllocateElements from VariableLengthVector).

I see, https://open.cdash.org/builds/10917349/dynamic_analysis/11341974 says:

[ RUN      ] MaskedAssignFixture.SetGetPrint
/codebuild/output/src1970974602/src/actions-runner/_work/SimpleITK/SimpleITK/ITK/Modules/Remote/SimpleITKFilters/test/itkMaskedAssignImageFilterGTest.cxx:139: Failure
Expected: filter->GetInput() doesn't throw an exception.
  Actual: it throws itk::ExceptionObject with description "/codebuild/output/src1970974602/src/actions-runner/_work/SimpleITK/SimpleITK/ITK/Modules/Core/Common/include/itkExceptionObject.h:261: in 'TTarget itk::itkDynamicCastInDebugMode(TSource) [with TTarget = const itk::Image<float, 3>*; TSource = const itk::DataObject*]':
ITK ERROR: Failed dynamic cast to PKN3itk5ImageIfLj3EEE object type = SimpleDataObjectDecorator".

@issakomi
Copy link
Member

issakomi commented Dec 29, 2025

Valgrind seems to be complaining

https://open.cdash.org/builds/10917349/dynamic_analysis

MaskedAssignFixture.SetGetPrint failed, I am talking about other tests with Valgring defects:

==16204==    at 0x484A2F3: operator new[](unsigned long) (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==16204==    by 0x426380: VariableLengthVector (itkVariableLengthVector.hxx:32)
==16204==    at 0x484A2F3: operator new[](unsigned long) (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==16204==    by 0x41FC94: itk::VariableLengthVector<unsigned char>::VariableLengthVector(unsigned int) (itkVariableLengthVector.hxx:32)
==16204==    by 0x42082D: itk::VariableLengthVector<unsigned char>::VariableLengthVector(itk::VariableLengthVector<unsigned char> const&) (itkVariableLengthVector.hxx:62)

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants