Reverted some "cleaning up" of the analyser.

Effectively reverting 4357d2b. Indeed, the code is less efficient: there are more `if` statements and we make many calls (~94% of the time) to AnalyserEquationAst::swapLeftAndRightChildren(). Also, if we were to do that cleaning up then we also need to clean in the case of a piecewise statement.
cellml · Dec 5, 2024 · e6583cf · e6583cf
1 parent b3fb67a
commit e6583cf
Show file tree

Hide file tree

Showing 73 changed files with 2,821 additions and 2,820 deletions.
diff --git a/src/analyser.cpp b/src/analyser.cpp
@@ -742,34 +742,36 @@ void Analyser::AnalyserImpl::analyseNode(const XmlNodePtr &node,
         //                 +-------------+
 
         auto childCount = mathmlChildCount(node);
-        AnalyserEquationAstPtr tempAst;
-        AnalyserEquationAstPtr astRightChild;
 
-        for (size_t i = childCount - 1; i > 0; --i) {
-            astRightChild = tempAst;
-            tempAst = AnalyserEquationAst::create();
+        analyseNode(mathmlChildNode(node, 0), ast, astParent, component, equation);
+
+        if (childCount >= 2) {
+            analyseNode(mathmlChildNode(node, 1), ast->mPimpl->mOwnedLeftChild, ast, component, equation);
+
+            if (childCount >= 3) {
+                AnalyserEquationAstPtr astRightChild;
+                AnalyserEquationAstPtr tempAst;
+
+                analyseNode(mathmlChildNode(node, childCount - 1), astRightChild, nullptr, component, equation);
+
+                for (auto i = childCount - 2; i > 1; --i) {
+                    tempAst = AnalyserEquationAst::create();
+
+                    analyseNode(mathmlChildNode(node, 0), tempAst, nullptr, component, equation);
+                    analyseNode(mathmlChildNode(node, i), tempAst->mPimpl->mOwnedLeftChild, tempAst, component, equation);
 
-            if (astRightChild != nullptr) {
-                if (i == childCount - 2) {
-                    astRightChild->swapLeftAndRightChildren();
-                    tempAst = astRightChild;
-                } else {
                     astRightChild->mPimpl->mParent = tempAst;
+
                     tempAst->mPimpl->mOwnedRightChild = astRightChild;
+                    astRightChild = tempAst;
                 }
-            }
 
-            if (i != childCount - 2) {
-                analyseNode(mathmlChildNode(node, 0), tempAst, nullptr, component, equation);
-            }
+                astRightChild->mPimpl->mParent = ast;
 
-            analyseNode(mathmlChildNode(node, i), tempAst->mPimpl->mOwnedLeftChild, tempAst, component, equation);
+                ast->mPimpl->mOwnedRightChild = astRightChild;
+            }
         }
 
-        analyseNode(mathmlChildNode(node, 0), tempAst, astParent, component, equation);
-
-        ast = tempAst;
-
         // Relational and logical operators.
 
     } else if (node->isMathmlElement("eq")) {

diff --git a/tests/analyser/analyser.cpp b/tests/analyser/analyser.cpp
@@ -898,8 +898,8 @@ TEST(Analyser, algebraicSystemWithThreeLinkedUnknownsWithOneExternalVariable)
     EXPECT_EQ(size_t(0), parser->issueCount());
 
     const std::vector<std::string> expectedIssues = {
-        "Variable 'z' in component 'my_algebraic_system' is computed more than once.",
         "Variable 'y' in component 'my_algebraic_system' is computed more than once.",
+        "Variable 'z' in component 'my_algebraic_system' is computed more than once.",
     };
 
     auto analyser = libcellml::Analyser::create();
@@ -954,8 +954,8 @@ TEST(Analyser, overconstrainedNlaSystem)
     EXPECT_EQ(size_t(0), parser->issueCount());
 
     const std::vector<std::string> expectedIssues = {
-        "Variable 'y' in component 'my_algebraic_system' is computed more than once.",
         "Variable 'x' in component 'my_algebraic_system' is computed more than once.",
+        "Variable 'y' in component 'my_algebraic_system' is computed more than once.",
     };
 
     auto analyser = libcellml::Analyser::create();

diff --git a/tests/analyser/analyserunits.cpp b/tests/analyser/analyserunits.cpp
@@ -883,8 +883,8 @@ TEST(AnalyserUnits, powerValues)
         "The units in 'eqnPi = pow(x, pi)' in component 'my_component' are not equivalent. 'eqnPi' is in 'second' while 'pow(x, pi)' is in 'second^3.14159'.",
         "The units in 'eqnInfinity = pow(x, infinity)' in component 'my_component' are not equivalent. 'eqnInfinity' is in 'second' while 'pow(x, infinity)' is in 'second^inf' (i.e. '10^nan x second^inf').",
         "The units in 'eqnNotanumber = pow(x, notanumber)' in component 'my_component' are not equivalent. 'eqnNotanumber' is in 'second' while 'pow(x, notanumber)' is in 'second^nan' (i.e. '10^nan x second^nan').",
-        "The type of variable 'u' in component 'my_component' is unknown.",
         "The type of variable 'eqnCoverage' in component 'my_component' is unknown.",
+        "The type of variable 'u' in component 'my_component' is unknown.",
         "The type of variable 'eqnCoverage2' in component 'my_component' is unknown.",
         "The type of variable 'eqnCoverage3' in component 'my_component' is unknown.",
     };

diff --git a/tests/bindings/javascript/analysermodel.test.js b/tests/bindings/javascript/analysermodel.test.js
@@ -64,7 +64,7 @@ describe("Analyser Model tests", () => {
     test('Checking Analyser Model variables related API.', () => {
         expect(am.variableCount()).toBe(18)
         expect(am.variables().size()).toBe(18)
-        expect(am.variable(2).variable().name()).toBe("i_L")
+        expect(am.variable(2).variable().name()).toBe("i_K")
     });
     test('Checking Analyser Model need* API.', () => {
         expect(am.needEqFunction()).toBe(false)

diff --git a/tests/bindings/python/test_analyser.py b/tests/bindings/python/test_analyser.py
@@ -152,12 +152,12 @@ def test_coverage(self):
 
         # Ensure coverage for AnalyserVariable.
 
-        av = am.variable(4)
+        av = am.variable(3)
 
         self.assertEqual(AnalyserVariable.Type.CONSTANT, av.type())
         self.assertEqual("constant", AnalyserVariable.typeAsString(av.type()))
         self.assertEqual("constant", AnalyserVariable_typeAsString(av.type()))
-        self.assertEqual(4, av.index())
+        self.assertEqual(3, av.index())
         self.assertIsNotNone(av.initialisingVariable())
         self.assertIsNotNone(av.variable())
         self.assertEqual(1, av.equationCount())

diff --git a/tests/coverage/coverage.cpp b/tests/coverage/coverage.cpp
@@ -623,9 +623,8 @@ TEST(Coverage, generator)
         EXPECT_NE(nullptr, analyserModel->state(i)->initialisingVariable());
     }
 
-    std::vector<size_t> nonNull = {0, 1, 5, 17, 178, 179, 181, 205, 206};
     for (size_t i = 0; i < analyserModel->variableCount(); ++i) {
-        if (std::find(nonNull.begin(), nonNull.end(), i) != nonNull.end()) {
+        if ((i == 1) || (i == 2) || (i == 6) || (i == 18) || (i == 179) || (i == 180) || (i == 182) || (i == 205) || (i == 206)) {
             EXPECT_TRUE(analyserModel->variable(i)->initialisingVariable() != nullptr);
         }
     }