STYLE: Remove private ObjectFactoryBase member RegisterInternal()#4228
Conversation
N-Dekker
force-pushed
the
Remove-ObjectFactoryBase-RegisterInternal
branch
from
7819efe to
9aecafd
Compare
N-Dekker
changed the title
RegisterInternal()RegisterInternal()
Replaced `clear` and `push_back` calls with a simple `std::list` assignment to `m_RegisteredFactories`.
`ObjectFactoryBase::RegisterInternal()` was only called by `Initialize()` anyway. Moved the code from `RegisterInternal()` into `Initialize()`, except from an `itkInitGlobalsMacro(PimplGlobals)` call which is no longer necessary.
N-Dekker
force-pushed
the
Remove-ObjectFactoryBase-RegisterInternal
branch
from
9aecafd to
d86e2f9
Compare
github-actions
Bot
added
the
type:Style
|
FYI, the last force-push has just split the changes from one commit to two, hoping to ease further processing. |
|
@blowekamp could you also review? |
| { | ||
| ObjectFactoryBase::RegisterInternal(); | ||
| // Guarantee that no internal factories have already been registered. | ||
| itkAssertInDebugAndIgnoreInReleaseMacro(m_PimplGlobals->m_RegisteredFactories.empty()); |
There was a problem hiding this comment.
This will only be guaranteed in debug, and will be ignored in release. Perhaps we should create a warning if it's not empty and then clear the registered factories? I generally assume there was a reason for line being there rather and it should be preserved.
There was a problem hiding this comment.
Thanks for your comment, Bradley. This PR is intended to only propose a style improvement, so I'd rather not change the behavior (in release). But I did have a look. The following commit made the assertion "debug-only": 302d53c "BUG: Prevent ObjectFactoryBase from possibly throwing an exception" by Brian Helba (@brianhelba) January 8, 2014. I wouldn't mind leaving it like this at the moment. 🤷
There was a problem hiding this comment.
Well if you want to keep the original behavior I'd keep the clear.
There was a problem hiding this comment.
Well if you want to keep the original behavior I'd keep the
clear.
Why? Please look at the original code again:
ITK/Modules/Core/Common/src/itkObjectFactoryBase.cxx
Lines 252 to 259 in 75b9967
m_PimplGlobals->m_RegisteredFactories.clear();
for (auto & internalFactory : m_PimplGlobals->m_InternalFactories)
{
m_PimplGlobals->m_RegisteredFactories.push_back(internalFactory);
}
Why not just doing a single std::list assignment instead, m_PimplGlobals->m_RegisteredFactories = m_PimplGlobals->m_InternalFactories?
There was a problem hiding this comment.
Yes, good point they are quite similar. This was incorrect justification for why I didn't think this PR was a refactor and not a style change.
There was a problem hiding this comment.
@blowekamp Cool, thanks for explaining! 👍 And thanks for double-checking!
blowekamp
left a comment
blowekamp
left a comment
There was a problem hiding this comment.
Looks like a reasonable refactor.
3 participants
ObjectFactoryBase::RegisterInternal()was only called byInitialize()anyway.Avoided a redundant
itkInitGlobalsMacro(PimplGlobals)call, and replacedclearandpush_backcalls by a simple assignment tom_RegisteredFactories.