Remove AllocateElements from VariableLengthVector by N-Dekker · Pull Request #5710 · InsightSoftwareConsortium/ITK

N-Dekker · 2025-12-23T11:09:59Z

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

ITK/Modules/Core/Common/include/itkVariableLengthVector.hxx

Lines 204 to 216 in d66197a

    
           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 · 2025-12-23T11:25:14Z

Modules/Core/Common/include/itkVariableLengthVector.hxx

-  : m_Data(nullptr)
+  : m_Data(new TValue[length])
 {
-  Reserve(length);


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.

blowekamp

These changes look good to me.

dzenanz

Looks good on a glance.

thewtex

Nice!

issakomi · 2025-12-24T19:54:27Z

Valgrind seems to be complaining

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

N-Dekker · 2025-12-29T21:22:46Z

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".

I think this is caused by ITKSimpleITKFilters PR COMP: Update GitHub Actions from ITKModuleTemplate ITKSimpleITKFilters#11 commit InsightSoftwareConsortium/ITKSimpleITKFilters@be21024 and addressed by ITKSimpleITKFilters PR Update tests for throwing exception with inputs ITKSimpleITKFilters#25 However, as @blowekamp remarked at Update tests for throwing exception with inputs ITKSimpleITKFilters#25 (comment), filter->GetInput() may or may not throw an exception, depending on whether or not it's a Debug build.

issakomi · 2025-12-29T22:56:02Z

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)

N-Dekker · 2025-12-30T12:19:59Z

Ah, thanks @issakomi I see now, when the copy-constructor of VariableLengthVector copies a VariableLengthVector of length zero, it calls VariableLengthVector(length) with length == 0 which initializes m_Data by new TValue[0]. Afterwards, the copy-constructor does m_Data = nullptr, so then it leaks new TValue[0].

I think it would be better if VariableLengthVector(length) would simply make m_Data null, when length is zero. Instead of doing new TValue[0]. Right?

Originally, a zero-length VariableLengthVector might still have its `m_Data` pointing to the heap, as it might have its memory allocated by `new TValue[length]`, with `length == 0`. A memory leak in `VariableLengthVector(unsigned int length)` was then introduced by pull request InsightSoftwareConsortium#5710 commit bb79801 "STYLE: Replace AllocateElements calls within VariableLengthVector" forgetting to `delete` memory that was allocated by `new TValue[0]`. As was reported by Mihail Isakov, who observed, pointing to the itk-valgrind results at https://open.cdash.org/builds/10917349/dynamic_analysis With this commit, m_Data should always be null when the VariableLengthVector is empty and manages its own memory (m_LetArrayManageMemory is `true`).

Originally, a zero-length VariableLengthVector might still have its `m_Data` pointing to the heap, as it might have its memory allocated by `new TValue[length]`, with `length == 0`. A memory leak in `VariableLengthVector(unsigned int length)` was then introduced by pull request InsightSoftwareConsortium#5710 commit bb79801 "STYLE: Replace AllocateElements calls within VariableLengthVector" forgetting to `delete` memory that was allocated by `new TValue[0]`. As was reported by Mihail Isakov, referring to the Valgrind results at https://open.cdash.org/builds/10917349/dynamic_analysis With this commit, m_Data should always be null when the VariableLengthVector is empty and manages its own memory (m_LetArrayManageMemory is `true`).

N-Dekker requested a review from blowekamp December 23, 2025 11:09

github-actions bot added the area:Core Issues affecting the Core module label Dec 23, 2025

N-Dekker commented Dec 23, 2025

View reviewed changes

N-Dekker added 2 commits December 23, 2025 13:27

STYLE: Replace AllocateElements calls within VariableLengthVector

bb79801

It appears unnecessary for VariableLengthVector to replace exceptions from `new T[n]` with ITK exceptions. This commit also simplified the `VariableLengthVector(length)` constructor.

STYLE: FUTURE REMOVE/deprecate VariableLengthVector::AllocateElements

f080d4f

Although AllocateElements is a public member function of VariableLengthVector, it was only used internally, by VariableLengthVector itself.

N-Dekker force-pushed the Remove-AllocateElements-from-VariableLengthVector branch from a222c4d to f080d4f Compare December 23, 2025 12:28

blowekamp approved these changes Dec 23, 2025

View reviewed changes

dzenanz reviewed Dec 23, 2025

View reviewed changes

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

thewtex approved these changes Dec 23, 2025

View reviewed changes

thewtex merged commit 86a2bbf into InsightSoftwareConsortium:main Dec 23, 2025
17 checks passed

N-Dekker mentioned this pull request Jan 2, 2026

COMP: Missing ITK_FUTURE_LEGACY_REMOVE in VariableLengthVector #5712

Merged

7 tasks

N-Dekker mentioned this pull request Jan 2, 2026

BUG: Make VariableLengthVector::m_Data null when its length is zero #5713

Merged

N-Dekker mentioned this pull request Mar 6, 2026

COMP: Make VariableLengthVector::AllocateElements ITK_FUTURE_DEPRECATED #5869

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Remove AllocateElements from VariableLengthVector#5710

Remove AllocateElements from VariableLengthVector#5710
thewtex merged 2 commits intoInsightSoftwareConsortium:mainfrom
N-Dekker:Remove-AllocateElements-from-VariableLengthVector

N-Dekker commented Dec 23, 2025

Uh oh!

N-Dekker Dec 23, 2025

Uh oh!

blowekamp left a comment

Uh oh!

dzenanz left a comment

Uh oh!

thewtex left a comment

Uh oh!

Uh oh!

issakomi commented Dec 24, 2025

Uh oh!

N-Dekker commented Dec 29, 2025

Uh oh!

issakomi commented Dec 29, 2025 •

edited

Loading

Uh oh!

N-Dekker commented Dec 30, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

	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.");
	}
	}

Uh oh!

Conversation

N-Dekker commented Dec 23, 2025

Uh oh!

N-Dekker Dec 23, 2025

Choose a reason for hiding this comment

Uh oh!

blowekamp left a comment

Choose a reason for hiding this comment

Uh oh!

dzenanz left a comment

Choose a reason for hiding this comment

Uh oh!

thewtex left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

issakomi commented Dec 24, 2025

Uh oh!

N-Dekker commented Dec 29, 2025

Uh oh!

issakomi commented Dec 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

N-Dekker commented Dec 30, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

issakomi commented Dec 29, 2025 •

edited

Loading