Add VariableLengthVector(length, value) constructor#5693
Conversation
github-actions
Bot
added
type:Testing
| return data.release(); | ||
| }() } | ||
| , m_NumElements{ length } | ||
| {} |
There was a problem hiding this comment.
This implementation looks reasonable.... but, I looks at the other implementation in the hxx file. Why is this in the header and not hxx file? And is this implementation is rather different that VariableLengthVector(unsigned int) which uses Reserve. Might be good to keep these is sync and not diverge too much?
Might even make since to do:
explicit VariableLengthVector(unsigned int length, const TValue & value) : VariableLengthVector(length) {
std::fill_n(m_Data, length, value);
}
There was a problem hiding this comment.
Thanks for your comments @blowekamp Is it preferable to move the implementation of this little constructor to the hxx file? VariableLengthVector does have some of its member function implementations in its h file, already:
ITK/Modules/Core/Common/include/itkVariableLengthVector.h
Lines 402 to 406 in 1096547
There was a problem hiding this comment.
Your suggestion is nice, but it may not be exception-safe:
explicit VariableLengthVector(unsigned int length, const TValue & value) : VariableLengthVector(length) {
std::fill_n(m_Data, length, value);
}
When std::fill_n(m_Data, length, value) throws an exception, is the data still deallocated?
Update: I just checked: your suggestion does not leak, when the body (the std::fill_n call) throws an exception. 👍
There was a problem hiding this comment.
There is some extra complication in the VariableLengthVector(unsigned int) construction... but there seems to be a lot of room for improvement in this class.
Thank you for working on it.
There was a problem hiding this comment.
@blowekamp Addressing your comment, please wait...
There was a problem hiding this comment.
There is some extra complication in the
VariableLengthVector(unsigned int)construction... but there seems to be a lot of room for improvement in this class.
Indeed! I think VariableLengthVector(unsigned int) should call AllocateElements, instead of Reserve, right? Or simply just do new TValue[length]!
There was a problem hiding this comment.
The utility of AllocateElements wrapping a new to throw an exception which requires additional memory allocation to perform ostringstream operations is certainly IMHO, questionable.
There was a problem hiding this comment.
I think the changes to the existing constructor is another PR.
N-Dekker
force-pushed
the
VariableLengthVector-constructor-with-length-and-value
branch
from
e5f6069 to
13477f5
Compare
Follow-up to pull request InsightSoftwareConsortium#2248 commit 73ce4ca "ENH: Array, OptimizerParameters constructors with size and initial value"
Replaced local `b` variables and `b.Fill` calls.
N-Dekker
force-pushed
the
VariableLengthVector-constructor-with-length-and-value
branch
from
13477f5 to
a912fac
Compare
Following pull request InsightSoftwareConsortium#5693 commit a912fac "STYLE: Do return `VariableLengthVector(length, value)` in NumericTraits"
5 participants
Proposed a
VariableLengthVector(unsigned int length, const TValue & value)constructor that creates aVariableLengthVectorof the specifiedlength, filled with the specifiedvaluefor each element.Added a use case: simplifying
NumericTraits<VariableLengthVector<T>>.ArrayandOptimizerParametersalready have such a constructor.