remove LegacyControllerMappingVisitor #3635
Conversation
Be-ing
force-pushed
the
remove_legacycontrollermappingvisitor
branch
from
6b13f08
to
97179ac
Compare
| // This immediately calls one of the two visit() methods below. | ||
| m_pMapping->accept(this); | ||
| m_pMidiMapping = dynamic_cast<LegacyMidiControllerMapping*>(pMapping.data()); | ||
| if (!m_pMidiMapping) { |
There was a problem hiding this comment.
Should this become VERIFY_OR_DEBUG_ASSERT? Otherwise please add a short comment that silently ignoring any other derived type here and resetting the member to nullptr when the dynamic cast fails is intended.
If the setMapping() is called twice first with the expected type (Midi) and then with another type (Hid) the internal pointer would be reset and the first invocation would get lost.
There was a problem hiding this comment.
I tried that initially but was asserting even though everything was working fine. A check could be added to the calling code to avoid that, but I'm trying to keep the changes as minimal as possible for now.
There was a problem hiding this comment.
That's fine. Just write a short comment about what you know. This knowledge might be helpful in the future.
The code looks suspicious to me without knowing the context or history. Usually you take a different route if a dynamic_cast fails. Here you (re-)set the member unconditionally before aborting.
There was a problem hiding this comment.
Okay, I added a comment. I may implement that debug assertion later after cleaning up more code. We'll see. First I need to clear the road before I can see the paths ahead.
This class increased coupling and made the code very confusing for no benefit.
Be-ing
force-pushed
the
remove_legacycontrollermappingvisitor
branch
from
97179ac
to
5421a3d
Compare
|
merge? |
|
Thank you for the quick review. I will remove ConstLegacyControllerMappingVisitor next. |
This class increased coupling and made the code very confusing for
no benefit.