remove ConstLegacyControllerMappingVisitor

[Be-ing]

This class increased coupling and made the code very confusing.

[uklotzde]

Consistently use LegacyControllerMappingPointer everywhere, not plain pointers. Your code already has memory leaks.

Change the typedef from QSharedPointer to std::shared_pointer and then you could use dynamic_pointer_cast instead of dynamic_cast.

http://www.cplusplus.com/reference/memory/dynamic_pointer_cast/

Alternatively use qSharedPointerDynamicCast but I would prefer the standard C++ route instead of dealing with custom Qt re-inventions if not strictly necessary.

[Be-ing]

I'll get rid of the QSharedPointer and use std. Might make the diff quite big though.

[uklotzde]

If the diff is too large then keep using QSharedPointer. Doesn't really matter.

[Be-ing]

Come to think of it, std::unique_ptr might be a better choice here to help enforce that the cloned object is not shared between threads.

[Be-ing]

Ugh, there's no equivalent of dynamic_pointer_cast for unique_ptr...

[uklotzde]

Maybe add a comment: // Downcast and take ownership on success

It's a shame that you need to write so much code and a comment to get this right. C++ 😩

[Be-ing]

🙈

[Be-ing]

Actually, a shared pointer is needed. DlgPrefController, ControllerInputMappingTableModel, and ControllerOutputMappingTableModel all share a pointer in the main thread.

[Be-ing]

void HidController::setMapping(std::unique_ptr<LegacyControllerMapping> pMapping) {
    m_pMapping = std::unique_ptr<LegacyHidControllerMapping>(
        dynamic_cast<LegacyHidControllerMapping*>(pMapping.get()));
    if (m_pMapping) {
        pMapping.release();
    }
}

This seems to double free or something? Forget it, I'll use shared pointers and DEBUG_ASSERT that use_count is 1. 🙈

[uklotzde]

Exactly oncy. It is freed later by m_pMapping if the dynamic_cast succeeded or immediately when pMapping goes out of scope. release() doesn't free.

[Be-ing]

I gave up trying to mix shared_ptr and unique_ptr. The best I could do was DEBUG_ASSERT that shared_ptr's use_count is 1. That took way more work than it should have.

[uklotzde]

I gave up trying to mix shared_ptr and unique_ptr. The best I could do was DEBUG_ASSERT that shared_ptr's use_count is 1. That took way more work than it should have.

This happens if you use the wrong abstractions like a shared pointer for everything because it is convenient without thinking about ownership and the consequences. This design was already broken before and don't need to be fixed right now.

[Be-ing]

had to change this to 2 because C++

wtf is this language 🙃

[uklotzde]

If you pass it with std::move it should (or might?) work as expected. This is what you would also need to do when using a std::unique_ptr.

[uklotzde]

Explicitly enforce the move on the caller site by turning the parameter type into an rvalue reference, i.e. std::shared_ptr<..>&&.

[uklotzde]

All ugly workarounds for missing move semantics and the reason why std::unique_ptr would be a better choice for the parameter type.

[Be-ing]

Casting that magic spell worked 🧙‍♂️

[Be-ing]

Maybe I could make a separate uniqueClone method for LegacyControllerMapping? Also ugly, but 🤷

[Be-ing]

Whatever, this works and CI passed. Let's move on.

[uklotzde]

The workaround is brittle but the use count assertion ensures that we will notice early if it does not work as intended. The code before this refactoring worked more or less by chance with implicit and hidden assumptions. Thanks for taking the effort to sort this out! LGTM

[Be-ing]

Thank you for review. I think I can start decoupling the legacy mapping system from Controller and MidiController now.