From 60c5c39ee5e92f4b7cc20f15b6980f20b6fd09f4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Janiszewski?= Date: Fri, 26 Jul 2019 08:24:12 +0200 Subject: [PATCH] Fix #563: Odd dependency cycle (#570) There is an odd dependency in CalcEngine. `CalculatorManager` inherits `ICalcDisplay` and implements a set of virtual calls it exposes, in particular `SetPrimaryDisplay`. https://github.com/microsoft/calculator/blob/2517854836fee634a273adf3be72371cdb51ddaa/src/CalcManager/Header%20Files/ICalcDisplay.h#L13 When setting a mode in `CalculatorManager`, e.g. https://github.com/microsoft/calculator/blob/2517854836fee634a273adf3be72371cdb51ddaa/src/CalcManager/CalculatorManager.cpp#L208 `this` (here: an instance of `CalculatorManager`) gets passed as an argument to the newly created `CCalcEngine` as `ICalcDisplay` pointer and the engine is stored as `unique_ptr` member field of `CalculatorManager`. In the destructor of `CalculatorManager`, a single function is called, `MemorizedNumberClearAll` which then invokes `ProcessCommand(IDC_MCLEAR)` on current engine, gets passed on to `CCalcEngine::ProcessCommandWorker`, to `DisplayNum`, to `CCalcEngine::SetPrimaryDisplay` and finally to `m_pCalcDisplay->SetPrimaryDisplay`, but here `m_pCalcDisplay` _was_ the instance of `CalculatorManager` that just got its destructor called. https://github.com/microsoft/calculator/blob/2517854836fee634a273adf3be72371cdb51ddaa/src/CalcManager/CalculatorManager.cpp#L46 https://github.com/microsoft/calculator/blob/2517854836fee634a273adf3be72371cdb51ddaa/src/CalcManager/CalculatorManager.cpp#L475 https://github.com/microsoft/calculator/blob/2517854836fee634a273adf3be72371cdb51ddaa/src/CalcManager/CEngine/scicomm.cpp#L87 https://github.com/microsoft/calculator/blob/2517854836fee634a273adf3be72371cdb51ddaa/src/CalcManager/CEngine/scicomm.cpp#L133 https://github.com/microsoft/calculator/blob/2517854836fee634a273adf3be72371cdb51ddaa/src/CalcManager/CEngine/scidisp.cpp#L124 https://github.com/microsoft/calculator/blob/2517854836fee634a273adf3be72371cdb51ddaa/src/CalcManager/CEngine/scicomm.cpp#L837 It will likely differ by implementation on how exactly, but the [standard suggests](http://eel.is/c++draft/class.cdtor#4) that will invoke the pure virtual `ICalcDisplay::SetPrimaryDisplay`. In case of GCC I believe the vtable is already destroyed by the time you enter dtor's body. There appears to be no reason to call `MemorizedNumberClearAll` in `CalculatorManager::~CalculatorManager()` because the calc manager and all its engines are going down anyway. Therefore, removing the call (and thus, the destructor which would then be empty). Fixes #563: Odd dependency cycle --- src/CalcManager/CalculatorManager.cpp | 9 --------- src/CalcManager/CalculatorManager.h | 1 - 2 files changed, 10 deletions(-) diff --git a/src/CalcManager/CalculatorManager.cpp b/src/CalcManager/CalculatorManager.cpp index e09d7d90..12df63bb 100644 --- a/src/CalcManager/CalculatorManager.cpp +++ b/src/CalcManager/CalculatorManager.cpp @@ -37,15 +37,6 @@ namespace CalculationManager CCalcEngine::InitialOneTimeOnlySetup(*m_resourceProvider); } - /// - /// Destructor for CalculatorManager - /// Ends two CCalcEngine - /// - CalculatorManager::~CalculatorManager() - { - this->MemorizedNumberClearAll(); - } - /// /// Call the callback function using passed in IDisplayHelper. /// Used to set the primary display value on ViewModel diff --git a/src/CalcManager/CalculatorManager.h b/src/CalcManager/CalculatorManager.h index 37406ed4..d34d2e35 100644 --- a/src/CalcManager/CalculatorManager.h +++ b/src/CalcManager/CalculatorManager.h @@ -103,7 +103,6 @@ namespace CalculationManager void MemoryItemChanged(unsigned int indexOfMemory) override; CalculatorManager(ICalcDisplay* displayCallback, IResourceProvider* resourceProvider); - ~CalculatorManager(); void Reset(bool clearMemory = true); void SetStandardMode();