From 476404b134970e7cfe39dbfd5edad0877f985fb3 Mon Sep 17 00:00:00 2001 From: Kris Katterjohn Date: Tue, 22 Dec 2020 17:45:17 -0500 Subject: [PATCH] Fix bogus translation of nested conditionals with elseif clauses This is an old bug that was present before Maxima 5.0. It doesn't appear that this bug was present in Macsyma because I haven't found any evidence that elseif existed in Macsyma. The translation of a conditional with another conditional directly nested in an elseif clause has been totally wrong. Using else-if (else if) instead of elseif would work fine. An example of some particularly bad behavior: if false then 'wtf1 elseif false then if true then 'wtf2 else 'wtf3 else correct was being incorrectly translated like if false then 'wtf1 else if true then 'wtf2 else 'wtf3 So this would yield wtf2 instead of correct. Some portions of code were not updated to correctly handle elseif when it was first introduced. See commits d89c9965 and 4cc510ca for some late fixes related to elseif. The problem in the translator has been due to an optimization for the else-if case. The check for when to apply the optimization was not particularly careful and the introduction of elseif caused the translator to incorrectly apply the optimization technique to elseif clauses like the one above. The optimization is still useful for the else-if case, so we're keeping it. We're just more careful about when to apply it now. No problems with the test suite or share test suite. tests/rtest_translator.mac runs as expected, with new tests. --- src/transl.lisp | 11 +++- tests/rtest_translator.mac | 126 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 135 insertions(+), 2 deletions(-) diff --git a/src/transl.lisp b/src/transl.lisp index 4c78c98d3..ada0c5d2b 100644 --- a/src/transl.lisp +++ b/src/transl.lisp @@ -1141,8 +1141,15 @@ APPLY means like APPLY.") mode (car dummy) nl (list dummy (translate-predicate (cadr form)))) (do ((l (cdddr form) (cddr l))) ((null l)) - (cond ((and (not (atom (cadr l))) (eq 'mcond (caaadr l))) - (setq l (cdadr l)))) + ; Optimize the else-if case: if we're at the else case at the end + ; and the body is just another conditional, then we just continue + ; directly with the clauses of the inner conditional instead of + ; nesting. + (when (and (null (cddr l)) + (eq (car l) t) + (consp (cadr l)) + (eq (caaadr l) 'mcond)) + (setq l (cdadr l))) (setq dummy (translate (cadr l)) mode (*union-mode mode (car dummy)) nl (cons dummy diff --git a/tests/rtest_translator.mac b/tests/rtest_translator.mac index 67e02d313..2d33211f2 100644 --- a/tests/rtest_translator.mac +++ b/tests/rtest_translator.mac @@ -1633,6 +1633,132 @@ block ([translate : false], (kill (mysignum1, mysignum2, foo, l1, l2), 0); 0; +/* Bogus translations of nested conditionals in elseif clauses + * + * The translation of a conditional with another conditional nested + * directly under an elseif clause was totally wrong. Using else-if + * ("else if") instead of elseif would work fine. + * + * + * We use the with_both_elseifs macro so we can test both elseif and + * else-if without having to duplicate portions of the tests below. + * Give this macro a conditional expression with elseifs and it will + * expand into a list: the first element is the same expression given + * to it (with elseifs), and the second element is that same expression + * rewritten to use else-ifs instead of elseifs. + */ + +(to_else_if (expr) := + if mapatom (expr) then + expr + else + block ([op : op (expr), args : args (expr)], + if op = "if" and length (args) > 4 then + funmake (op, map ('to_else_if, append (firstn (args, 2), [true, funmake (op, rest (args, 2))]))) + else + funmake (op, map ('to_else_if, args))), + with_both_elseifs (expr) ::= + buildq ([expr, texpr : to_else_if (expr)], + [expr, texpr]), + 0); +0; + +block ([translate : false], + foo () := + with_both_elseifs ( + if false then + 'lose1 + elseif false then + 'lose2 + elseif false then + if true then + 'lose3 + else + 'lose4 + else + 'win), + + /* l1: foo is interpreted */ + l1 : foo (), + + translate_or_lose (foo), + + /* l2: foo is translated + * + * foo used to give lose3 instead of win in the elseif case. + */ + l2 : foo (), + + [is (l1 = l2), + l2]); +[true, + ['win, 'win]]; + +block ([translate : false], + /* There is nothing special about bar here. This is just some + * function that has several branches with nested conditionals. + */ + bar (x) := + with_both_elseifs ( + if x > 5 then + if x > 7 then + 'more_than_seven + elseif x > 6 then + 'seven + else + 'six + elseif x > 2 then + if x > 4 then + 'five + elseif x > 3 then + 'four + else + 'three + elseif x >= 0 then + if x > 1 then + 'two + elseif x > 0 then + 'one + else + 'zero + else + 'negative), + + /* We test bar with the integers -2 to 9 */ + inputs : makelist (k, k, -2, 9), + + /* l1: bar is interpreted */ + l1 : map (bar, inputs), + + translate_or_lose (bar), + + /* l2: bar is translated + * + * bar used to give incorrect results in the elseif case for every + * number less than or equal to 2 (which means we got incorrect + * results for the integers -2 to 2 in this test). + */ + l2 : map (bar, inputs), + + [is (l2 = l1), + l2]); +[true, + [['negative, 'negative], + ['negative, 'negative], + ['zero, 'zero], + ['one, 'one], + ['two, 'two], + ['three, 'three], + ['four, 'four], + ['five, 'five], + ['six, 'six], + ['seven, 'seven], + ['more_than_seven, 'more_than_seven], + ['more_than_seven, 'more_than_seven]]]; + +(kill (foo, bar, l1, l2, inputs, to_else_if, with_both_elseifs), 0); +0; + -- 2.11.4.GIT