STYLE: Update comments and parameter in the ImportImageContainer #4360
Conversation
github-actions
bot
added
area:Core
issakomi
changed the title
| // Encapsulate all image memory allocation here to throw an | ||
| // exception when memory allocation fails even when the compiler | ||
| // does not do this by default. |
There was a problem hiding this comment.
I agree with you about removing these lines as we may assume nowadays that an exception is thrown whenever new fails to allocate the memory allocation, thanks. Just wondering if you really intended to include this change with your commit, as it's a bit of a different topic!
If we may indeed assume that new TElement[size] never returns null (as it will just return a bad_alloc exception, in case of a failure), the code may be somewhat simplified. 😃 But that would certainly be beyond the scope this commit, so you don't need to address it here. For the record, we did already assume before that new char[N] never returns null: 7482c80 (September 15, 2022)
There was a problem hiding this comment.
Thanks! If you like I can put this in a separate commit or mention in the commit message. Honestly, I have forgotten to write about it in the commit message. Yes, the text seems to be not precise, it is not a preference of a compiler, of course, default new always throws std::bad_alloc. About this catch (...): probably theoretically (but unlikely) could be possible to catch an exception from a default constructor. There is similar code in itkVariableLengthVector.hxx with a better comment. If you like I could add it too.
There was a problem hiding this comment.
You could indeed shorten the AllocateElements implementation to something like itkVariableLengthVector.hxx, removing the if (!data) test. If you would want to do so, please make it a separate commit, or even a separate PR. But you could also leave it for now. It's up to you!
|
Some very minor remarks (not a show-stopper), regarding the commit message:
|
Thanks! Yes, I shall address comments. I am setting temporary to draft. |
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
issakomi
changed the title
|
I have updated only the message and the comment. I have added some information about removed comment about 'new', There is still one commit, I hope it is OK, the name is "Update comments ...", probably it is applicable. |
issakomi
changed the title
issakomi
changed the title
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