don't do too much optimization for logical or

diff --git a/src/lib/unlang/xlat_purify.c b/src/lib/unlang/xlat_purify.c
index c4b14a40a3f50f89c897b8c519d6a7df95063a33..82f242cf24efda68f59bcbf9f43c00b37ff0bca8 100644 (file)
--- a/src/lib/unlang/xlat_purify.c
+++ b/src/lib/unlang/xlat_purify.c
@@ -241,41 +241,75 @@ static bool is_truthy(xlat_exp_t *node, bool *out)
  *     Do some optimizations.
  *
  */
-static xlat_exp_t *logical_peephole_optimize(xlat_exp_t *lhs, fr_token_t op, xlat_exp_t *rhs)
+static xlat_exp_t *peephole_optimize_lor(xlat_exp_t *lhs,  xlat_exp_t *rhs)
 {
        bool value;
 
        /*
-        *      @todo - check for tail of LHS
-        *              && tail is truthy, then remove tail, and call ourselves recursively
-        *              if there's a new node, it becomes the new tail.  Otherwise
-        *              we append the rhs to the lhs args.
+        *      LHS isn't truthy, we can't do anything.  If the LHS
+        *      passes, we return the value of the LHS.
         *
-        *      lhs->call.args->flags.can_purify |= rhs->flags.can_purify | rhs->flags.pure;
-        *      lhs->flags.can_purify = lhs->call.args->flags.can_purify;
+        *      FOO || ... --> FOO || ...
         */
        if (!is_truthy(lhs, &value)) {
-               xlat_exp_t *tmp;
-
-               if (!is_truthy(rhs, &value)) return NULL;
+               /*
+                *      FOO || 0 --> FOO much of the time
+                *      FOO || 1 --> FOO much of the time
+                *
+                *      @todo - if the LHS is a function AND the LHS returns a boolean, THEN we can optimized
+                *      the "FOO || 1" case to just "1".
+                */
+               return NULL;
+       }
 
-               tmp = lhs;
-               lhs = rhs;
-               rhs = tmp;
+       /*
+        *      1 || FOO   --> 1
+        *      0 || FOO   --> FOO
+        */
+       if (value) {
+               talloc_free(rhs);
+               return lhs;
        }
 
+       talloc_free(lhs);
+       return rhs;
+}
+
+
+/*
+ *     Do some optimizations.
+ *
+ */
+static xlat_exp_t *peephole_optimize_land(xlat_exp_t *lhs, xlat_exp_t *rhs)
+{
+       bool value;
+
        /*
-        *      1 && FOO   --> FOO
-        *      0 && FOO   --> 0
-        *      FOO && BAR --> FOO && BAR
+        *      LHS isn't truthy
+        *
+        *      FOO && ... --> FOO && ...
         */
+       if (!is_truthy(lhs, &value)) {
+               /*
+                *      FOO && 0 --> 0
+                *      FOO && 1 --> FOO
+                */
+               if (!is_truthy(rhs, &value)) return NULL;
+
+               if (!value) {
+                       talloc_free(lhs);
+                       return rhs;
+               }
+
+               talloc_free(rhs);
+               return lhs;
+       }
 
        /*
-        *      1 || FOO   --> 1
-        *      0 || FOO   --> FOO
-        *      FOO || BAR --> FOO || BAR
+        *      0 && FOO   --> 0
+        *      1 && FOO   --> FOO
         */
-       if (value == (op != T_LAND)) {
+       if (!value) {
                talloc_free(rhs);
                return lhs;
        }
@@ -284,7 +318,6 @@ static xlat_exp_t *logical_peephole_optimize(xlat_exp_t *lhs, fr_token_t op, xla
        return rhs;
 }
 
-
 /*
  *     Do peephole optimizations.
  */
@@ -350,7 +383,6 @@ static int binary_peephole_optimize(TALLOC_CTX *ctx, xlat_exp_t **out, xlat_exp_
        /*
         *      The tmpl_tokenize code takes care of resolving the data if there's a cast.
         */
-
        lhs_box = xlat_value_box(lhs);
        if (!lhs_box) return 0;
 
@@ -374,10 +406,20 @@ static int binary_peephole_optimize(TALLOC_CTX *ctx, xlat_exp_t **out, xlat_exp_
 
 int xlat_purify_op(TALLOC_CTX *ctx, xlat_exp_t **out, xlat_exp_t *lhs, fr_token_t op, xlat_exp_t *rhs)
 {
-       if ((op == T_LAND) || (op == T_LOR)) {
+       if (op == T_LOR) {
+               xlat_exp_t *node;
+
+               node = peephole_optimize_lor(lhs, rhs);
+               if (!node) return 0;
+
+               *out = node;
+               return 1;
+       }
+
+       if (op == T_LAND) {
                xlat_exp_t *node;
 
-               node = logical_peephole_optimize(lhs, op, rhs);
+               node = peephole_optimize_land(lhs, rhs);
                if (!node) return 0;
 
                *out = node;

diff --git a/src/tests/keywords/logical-or-assign b/src/tests/keywords/logical-or-assign
new file mode 100644 (file)
index 0000000..3c0fc43
--- /dev/null
+++ b/src/tests/keywords/logical-or-assign
@@ -0,0 +1,13 @@
+string foo
+
+&Acct-Session-Time = 30
+
+#
+#  Test for short-circuit logical optimizations
+#
+&foo = %{&Acct-Session-Time || 'NULL'}
+
+if !(&foo == "30") {
+       test_fail
+}
+success

diff --git a/src/tests/unit/condition/base.txt b/src/tests/unit/condition/base.txt
index 13d657fbe3db8d616352e23334427cfa81187454..afe19da23433b4fef163dad240b0a908f30cc24d 100644 (file)
--- a/src/tests/unit/condition/base.txt
+++ b/src/tests/unit/condition/base.txt
@@ -657,8 +657,18 @@ match false
 condition (&User-Name == "bob") && (false)
 match false
 
+#
+#  This could return a string of the User-Name, *or* a boolean true.
+#
+condition &User-Name || true
+match (&User-Name || true)
+
+#
+#  The evaluator does not know that the LHS returns a boolean, so it
+#  can't optimize it.
+#
 condition (&User-Name == "bob") || (true)
-match true
+match ((&User-Name == "bob") || true)
 
 #
 #  A && (B || C) is not the same as (A && B) || C, for 0/1/1
@@ -670,4 +680,4 @@ condition (&User-Name == "bob") && ((&User-Password == "bob") || &EAP-Message)
 match ((&User-Name == "bob") && ((&User-Password == "bob") || &EAP-Message))
 
 count
-match 305
+match 307