Avoid new and delete for internal data members of ObjectFactoryBase#3641
Conversation
Removed unnecessary dynamic memory allocation from internal factory lists.
N-Dekker
force-pushed
the
ObjectFactoryBase-unique_ptr
branch
from
33ffab5 to
09f6f8f
Compare
N-Dekker
changed the title
new and delete` for internal data members of ObjectFactoryBase
N-Dekker
changed the title
new and delete` for internal data members of ObjectFactoryBasenew and delete for internal data members of ObjectFactoryBase
dzenanz
left a comment
dzenanz
left a comment
There was a problem hiding this comment.
Did you figure out why heap allocation was used before?
| { | ||
| itk::ObjectFactoryBase::UnRegisterAllFactories(); | ||
| if (m_InternalFactories) | ||
| for (auto & m_InternalFactorie : m_InternalFactories) |
There was a problem hiding this comment.
English spelling is Factory, not Factorie 😄
There was a problem hiding this comment.
Indeed @dzenanz The term m_InternalFactorie appears to be introduced with a code refactorieng ;-) PR #1628 commit 7f2c44c:
ITK/Modules/Core/Common/src/itkObjectFactoryBase.cxx
Lines 109 to 112 in 7f2c44c
Anyway, I'd rather not include fixes of old typos with this PR, though. These can be fixed later, IMhO. Unless you insist...
There was a problem hiding this comment.
My check with
cd git/ITK
egrep -Ri 'factorie([^s]|$)'shows the misspelling "factorie" occurs only in this variable in this file, on exactly two lines of code.
There was a problem hiding this comment.
I thought you are introducing this typo in this commit, due to Dutch spelling being Factorie 😄
Of course, it can be fixed later. Maybe a new STYLE commit as part of this PR?
There was a problem hiding this comment.
@Leengit Thanks, but then you may also try for (const auto & m_ for similar typo's. 😺 (Remember we use the m_ prefix only for member variables.) Anyway, I'd rather defer such typo fixes to another PR, someone else, possibly.... unless you click "Request changes"!
There was a problem hiding this comment.
FWIW,
egrep -Ri 'for ?\((const )?auto ?&? ?[a-z_]*ie[a-z_]* 'returns only the case we know about.
(Edited to check for optional use of const and optional use of &.)
There was a problem hiding this comment.
OK, but for (const auto & m_ does occur twice anyway.
There was a problem hiding this comment.
For the issue of whether the looping variable should properly begin with m_, I find a total of three instances with egrep -Ri 'for ?\((const )?auto ?&? ?m_[a-z_]* ?':
Modules/IO/GDCM/src/itkGDCMSeriesFileNames.cxx: for (const auto & m_InputFileName : m_InputFileNames)
Modules/Core/Common/src/itkObjectFactoryBase.cxx: for (auto & m_InternalFactorie : *m_InternalFactories)
Modules/Core/Mesh/include/itkConnectedRegionsMeshFilter.hxx: for (const auto & m_RegionSize : m_RegionSizes)There was a problem hiding this comment.
@Leengit Would you possibly like to rename those three looping variables for us? 😃
@dzenanz Good question 🤔 It appears that ITK/Code/Common/itkObjectFactoryBase.h Line 163 in e31b986 You may ask Bill, but you know, at that time, raw pointers and manual memory management were very common! Everyone was using raw pointers around 2000! Maybe that could explain why...? |
|
STL was not consistently implemented in all the compilers back then, so that might be the reason. |
`ObjectFactoryBase::InitializeFactoryList()` has become unnecessary from pull request InsightSoftwareConsortium#3641 commit 48d0cb0 "STYLE: Remove pointer indirection from lists of ObjectFactoryBasePrivate"
`ObjectFactoryBase::InitializeFactoryList()` has become unnecessary from pull request InsightSoftwareConsortium#3641 commit 48d0cb0 "STYLE: Remove pointer indirection from lists of ObjectFactoryBasePrivate"
`ObjectFactoryBase::InitializeFactoryList()` has become unnecessary from pull request InsightSoftwareConsortium#3641 commit cf4e435 "STYLE: Remove pointer indirection from lists of ObjectFactoryBasePrivate"
3 participants
No description provided.