From 7ac750f7e580ba86c141b6f8d810891693024231 Mon Sep 17 00:00:00 2001
From: Daniel Belcher <dabelc@microsoft.com>
Date: Tue, 9 Apr 2019 14:12:28 -0700
Subject: [PATCH] Fix invalid check of m_precedenceOpCount (#298)

The conditional m_precedenceOpCount >= 0 was always true because m_precendenceOpCount is an unsigned type. Update the conditional to simply be true and rely on a break statement in the loop. Although this member variable used to be a signed type, in practice, the value was never less than 0.

How changes were validated:
Manual. Unit tests pass locally.
---
 src/CalcManager/CEngine/scicomm.cpp       | 91 +++++++++++++----------
 src/CalcManager/Header Files/CalcEngine.h |  1 +
 2 files changed, 52 insertions(+), 40 deletions(-)

diff --git a/src/CalcManager/CEngine/scicomm.cpp b/src/CalcManager/CEngine/scicomm.cpp
index f17938b..022bceb 100644
--- a/src/CalcManager/CEngine/scicomm.cpp
+++ b/src/CalcManager/CEngine/scicomm.cpp
@@ -463,45 +463,12 @@ void CCalcEngine::ProcessCommandWorker(OpCode wParam)
             m_HistoryCollector.AddOpndToHistory(m_numberString, m_currentVal);
         }
 
-        do {
-
-            if (m_nOpCode) /* Is there a valid operation around?        */
-            {
-                /* If this is the first EQU in a string, set m_holdVal=m_currentVal */
-                /* Otherwise let m_currentVal=m_holdVal.  This keeps m_currentVal constant */
-                /* through all EQUs in a row.                     */
-                if (m_bNoPrevEqu)
-                {
-                    m_holdVal = m_currentVal;
-                }
-                else
-                {
-                    m_currentVal = m_holdVal;
-                    DisplayNum(); // to update the m_numberString
-                    m_HistoryCollector.AddBinOpToHistory(m_nOpCode, false);
-                    m_HistoryCollector.AddOpndToHistory(m_numberString, m_currentVal); // Adding the repeated last op to history
-                }
-
-                // Do the current or last operation.
-                m_currentVal = DoOperation(m_nOpCode, m_currentVal, m_lastVal);
-                m_nPrevOpCode = m_nOpCode;
-                m_lastVal = m_currentVal;
-
-                /* Check for errors.  If this wasn't done, DisplayNum */
-                /* would immediately overwrite any error message.     */
-                if (!m_bError)
-                    DisplayNum();
-
-                /* No longer the first EQU.                       */
-                m_bNoPrevEqu = false;
-            }
-            else if (!m_bError)
-                DisplayNum();
-
-            if (m_precedenceOpCount == 0 || !m_fPrecedence)
-                break;
-
-            m_nOpCode = m_nPrecOp[--m_precedenceOpCount];
+        // Evaluate the precedence stack.
+        ResolveHighestPrecedenceOperation();
+        while (m_fPrecedence && m_precedenceOpCount > 0)
+        {
+            m_precedenceOpCount--;
+            m_nOpCode = m_nPrecOp[m_precedenceOpCount];
             m_lastVal = m_precedenceVals[m_precedenceOpCount];
 
             // Precedence Inversion check
@@ -514,7 +481,9 @@ void CCalcEngine::ProcessCommandWorker(OpCode wParam)
             m_HistoryCollector.PopLastOpndStart();
 
             m_bNoPrevEqu = true;
-        } while (m_precedenceOpCount >= 0);
+
+            ResolveHighestPrecedenceOperation();
+        }
 
         if (!m_bError)
         {
@@ -789,6 +758,48 @@ void CCalcEngine::ProcessCommandWorker(OpCode wParam)
 
 }
 
+// Helper function to resolve one item on the precedence stack.
+void CCalcEngine::ResolveHighestPrecedenceOperation()
+{
+    // Is there a valid operation around?
+    if (m_nOpCode)
+    {
+        // If this is the first EQU in a string, set m_holdVal=m_currentVal 
+        // Otherwise let m_currentVal=m_holdVal.  This keeps m_currentVal constant 
+        // through all EQUs in a row.
+        if (m_bNoPrevEqu)
+        {
+            m_holdVal = m_currentVal;
+        }
+        else
+        {
+            m_currentVal = m_holdVal;
+            DisplayNum(); // to update the m_numberString
+            m_HistoryCollector.AddBinOpToHistory(m_nOpCode, false);
+            m_HistoryCollector.AddOpndToHistory(m_numberString, m_currentVal); // Adding the repeated last op to history
+        }
+
+        // Do the current or last operation.
+        m_currentVal = DoOperation(m_nOpCode, m_currentVal, m_lastVal);
+        m_nPrevOpCode = m_nOpCode;
+        m_lastVal = m_currentVal;
+
+        // Check for errors.  If this wasn't done, DisplayNum
+        // would immediately overwrite any error message.
+        if (!m_bError)
+        {
+            DisplayNum();
+        }
+
+        // No longer the first EQU.
+        m_bNoPrevEqu = false;
+    }
+    else if (!m_bError)
+    {
+        DisplayNum();
+    }
+}
+
 // CheckAndAddLastBinOpToHistory
 //
 //  This is a very confusing helper routine to add the last entered binary operator to the history. This is expected to
diff --git a/src/CalcManager/Header Files/CalcEngine.h b/src/CalcManager/Header Files/CalcEngine.h
index 0ad8252..d9550a1 100644
--- a/src/CalcManager/Header Files/CalcEngine.h	
+++ b/src/CalcManager/Header Files/CalcEngine.h	
@@ -129,6 +129,7 @@ private:
 
 private:
     void ProcessCommandWorker(OpCode wParam);
+    void ResolveHighestPrecedenceOperation();
     void HandleErrorCommand(OpCode idc);
     void HandleMaxDigitsReached();
     void DisplayNum(void);