Re: stupid Matlab-style short-circuit behavior for | and &

[John W. Eaton]

On  7-Oct-2010, Jaroslav Hajek wrote:

| I was thinking about sth simple like
| 
| bool
| tree_binary_expression::is_logically_true (const char *warn_for,
|                                            bool bd_short_circuit)
| {
|   bool retval = false;
|   if (braindamage_short_circuit
|       && (etype == octave_value::op_el_and || etype == 
octave_value::op_el_or))
|     {
|        retval = op_lhs->is_logically_true (warn_for, bd_short_circuit);
|        if (! error_state && retval != (etype == octave_value::op_el_and))
|          retval = op_rhs->is_logically_true (warn_for, bd_short_circuit);
|     }
|   else
|     retval = tree_expression::is_logically_true (warn_for, bd_short_circuit);
| 
|   return retval;
| }

OK, but it is not quite this simple.  The short-circuit behavior only
happens if op_lhs evaluates to a scalar, so we can't just call
is_logically_true here, can we?  I think we need to call rvalue1 and
see if the result is a scalar, then if it is, do the short-circuit
evaluation.  Since op_lhs could itself be an | or & expression, we
need to pass bd_short_circuit to rvalue1.  Or we need some way to tell
is_logically_true to tell us that the object is logically true and also
that it evaluated to a scalar.  Or am I missing something?  Here is the
logic that is required (maybe it could be written more concisely, but
this way of expressing it is clear to me):

  if (Vdo_braindead_shortcircuit_evaluation
      && eligible_for_braindead_shortcircuit)
    {
      if (op_lhs)
        {
          octave_value a = op_lhs->rvalue1 ();

          if (! error_state)
            {
              if (a.ndims () == 2 && a.rows () == 1 && a.columns () == 1)
                {
                  bool result = false;

                  bool a_true = a.is_true ();

                  if (! error_state)
                    {
                      if (a_true)
                        {
                          if (etype == octave_value::op_el_or)
                            {
                              result = true;
                              goto done;
                            }
                        }
                      else
                        {
                          if (etype == octave_value::op_el_and)
                            goto done;
                        }

                      if (op_rhs)
                        {
                          octave_value b = op_rhs->rvalue1 ();

                          if (! error_state)
                            result = b.is_true ();
                        }

                    done:

                      if (! error_state)
                        return octave_value (result);
                    }
                }
            }
        }
    }

I'm also attaching a version using your suggestion, but I'm not sure
how to limit it to the case of the LHS being a scalar.  If you or
someone else can't see an easy way to do that, I'll just go with
something based on my first patch.

Thanks,

jwe

# HG changeset patch
# User John W. Eaton <address@hidden>
# Date 1286483833 14400
# Node ID fd0025a7d49b372f0d75056bd08cbcffecbbfba7
# Parent  2beacd515e09d41cac8c82e7680e0080252ac848
Matlab compatible short-circuit behavior for & and | operators

diff --git a/src/ChangeLog b/src/ChangeLog
--- a/src/ChangeLog
+++ b/src/ChangeLog
@@ -1,3 +1,25 @@
+2010-10-07  John W. Eaton  <address@hidden>
+            Jaroslav Hajek  <address@hidden>
+
+       * octave.cc (maximum_braindamage): Set
+       do_braindead_shortcircuit_evaluation to true.
+       * pt-eval.cc (Vdo_braindead_shortcircuit_evaluation): New
+       static variable.
+       (Fdo_braindead_shortcircuit_evaluation): New function.
+       (tree_evaluator::visit_if_command_list,
+       tree_evaluator::visit_while_command): Pass
+       Vdo_braindead_shortcircuit_evaluation to
+       tree_expression::is_logically_true method.
+
+       * oct-parse.yy (if_cmd_list1, elseif_clause, loop_command):
+       Warn about braindead Matlab-style short-circuit behavior in IF
+       and WHILE conditions.
+       * pt-binop.h
+       (tree_binary_expression::maybe_warn_braindead_shortcircuit):
+       New function.
+       * pt-exp.h (tree_expression::maybe_warn_braindead_shortcircuit):
+       New virtual function.
+
 2010-10-07  Rik <address@hidden>
 
        * DLD-FUNCTIONS/conv2.cc (convn): Update docstring.  Add 1 new test.
diff --git a/src/oct-parse.yy b/src/oct-parse.yy
--- a/src/oct-parse.yy
+++ b/src/oct-parse.yy
@@ -1066,7 +1066,11 @@
                 ;
 
 if_cmd_list1    : expression opt_sep opt_list
-                  { $$ = start_if_command ($1, $3); }
+                  {
+                    $1->maybe_warn_braindead_shortcircuit 
(curr_fcn_file_full_name);
+
+                    $$ = start_if_command ($1, $3);
+                  }
                 | if_cmd_list1 elseif_clause
                   {
                     $1->append ($2);
@@ -1075,7 +1079,11 @@
                 ;
 
 elseif_clause   : ELSEIF stash_comment opt_sep expression opt_sep opt_list
-                  { $$ = make_elseif_clause ($1, $4, $6, $2); }
+                  {
+                    $4->maybe_warn_braindead_shortcircuit 
(curr_fcn_file_full_name);
+
+                    $$ = make_elseif_clause ($1, $4, $6, $2);
+                  }
                 ;
 
 else_clause     : ELSE stash_comment opt_sep opt_list
@@ -1129,6 +1137,8 @@
 
 loop_command    : WHILE stash_comment expression opt_sep opt_list END
                   {
+                    $3->maybe_warn_braindead_shortcircuit 
(curr_fcn_file_full_name);
+
                     if (! ($$ = make_while_command ($1, $3, $5, $6, $2)))
                       ABORT_PARSE;
                   }
diff --git a/src/octave.cc b/src/octave.cc
--- a/src/octave.cc
+++ b/src/octave.cc
@@ -569,6 +569,7 @@
   bind_internal_variable ("confirm_recursive_rmdir", false);
   bind_internal_variable ("crash_dumps_octave_core", false);
   bind_internal_variable ("default_save_options", "-mat-binary");
+  bind_internal_variable ("do_braindead_shortcircuit_evaluation", true);
   bind_internal_variable ("fixed_point_format", true);
   bind_internal_variable ("history_timestamp_format_string",
                          "%%-- %D %I:%M %p --%%");
diff --git a/src/pt-binop.cc b/src/pt-binop.cc
--- a/src/pt-binop.cc
+++ b/src/pt-binop.cc
@@ -33,7 +33,50 @@
 #include "pt-walk.h"
 
 // Binary expressions.
- 
+
+bool
+tree_binary_expression::is_logically_true (const char *warn_for,
+                                           bool braindead_shortcircuit)
+{
+  bool retval = false;
+
+  if (braindead_shortcircuit
+      && (etype == octave_value::op_el_and || etype == octave_value::op_el_or))
+    {
+      bool a_true = op_lhs->is_logically_true (warn_for, 
braindead_shortcircuit);
+
+      if (! error_state)
+        {
+          if (a_true)
+            {
+              bool done = false;
+
+              if (etype == octave_value::op_el_or)
+                {
+                  retval = true;
+                  done = true;
+                }
+              else
+                {
+                  if (etype == octave_value::op_el_and)
+                    done = true;
+                }
+
+              if (! done && op_rhs)
+                {
+                  bool b_true = op_rhs->is_logically_true (warn_for, 
braindead_shortcircuit);
+                  if (! error_state)
+                    retval = b_true;
+                }
+            }
+        }
+    }
+  else
+    retval = tree_expression::is_logically_true (warn_for);
+
+  return retval;
+}
+
 octave_value_list
 tree_binary_expression::rvalue (int nargout)
 {
diff --git a/src/pt-binop.h b/src/pt-binop.h
--- a/src/pt-binop.h
+++ b/src/pt-binop.h
@@ -60,6 +60,25 @@
       delete op_rhs;
     }
 
+  void maybe_warn_braindead_shortcircuit (const std::string& file)
+    {
+      if (etype == octave_value::op_el_and
+          || etype == octave_value::op_el_or)
+        {
+          if (file.empty ())
+            warning_with_id ("possible-matlab-short-circuit-operator",
+                             "possible Matlab-style short-circuit operator at 
line %d, column %d",
+                             line (), column ());
+          else
+            warning_with_id ("possible-matlab-short-circuit-operator",
+                             "%s: possible Matlab-style short-circuit operator 
at line %d, column %d",
+                             file.c_str (), line (), column ());
+
+          op_lhs->maybe_warn_braindead_shortcircuit (file);
+          op_rhs->maybe_warn_braindead_shortcircuit (file);
+        }
+    }
+
   bool has_magic_end (void) const
     {
       return ((op_lhs && op_lhs->has_magic_end ())
@@ -68,6 +87,8 @@
 
   bool is_binary_expression (void) const { return true; }
 
+  bool is_logically_true (const char *warn_for, bool braindead_shortcircuit);
+
   bool rvalue_ok (void) const { return true; }
 
   octave_value rvalue1 (int nargout = 1);
diff --git a/src/pt-eval.cc b/src/pt-eval.cc
--- a/src/pt-eval.cc
+++ b/src/pt-eval.cc
@@ -66,6 +66,11 @@
 // semicolon has been appended to each statement).
 static bool Vsilent_functions = false;
 
+// TRUE means we implement braindead short-circuit behavior for | and &
+// expressions in the conditions of IF or WHILE statements, for
+// compatibility with Matlab.
+static bool Vdo_braindead_shortcircuit_evaluation;
+
 // Normal evaluator.
 
 void
@@ -563,7 +568,8 @@
       if (debug_mode && ! tic->is_else_clause ())
         do_breakpoint (tic->is_breakpoint ());
 
-      if (tic->is_else_clause () || expr->is_logically_true ("if"))
+      if (tic->is_else_clause ()
+          || expr->is_logically_true ("if", 
Vdo_braindead_shortcircuit_evaluation))
         {
           if (! error_state)
             {
@@ -1032,7 +1038,7 @@
       if (debug_mode)
         do_breakpoint (cmd.is_breakpoint ());
 
-      if (expr->is_logically_true ("while"))
+      if (expr->is_logically_true ("while", 
Vdo_braindead_shortcircuit_evaluation))
         {
           tree_statement_list *loop_body = cmd.body ();
 
@@ -1206,3 +1212,27 @@
 {
   return SET_INTERNAL_VARIABLE (silent_functions);
 }
+
+DEFUN (do_braindead_shortcircuit_evaluation, args, nargout,
+  "-*- texinfo -*-\n\
address@hidden  {Built-in Function} address@hidden =} 
do_braindead_shortcircuit_evaluation ()\n\
address@hidden  {Built-in Function} address@hidden =} 
do_braindead_shortcircuit_evaluation (@var{new_val})\n\
+Query or set the internal variable that controls whether Octave will\n\
+do short-circuit evaluation of @samp{|} and @samp{&} operators inside the\n\
+conditions of if or while statements.\n\
+\n\
+This feature is only provided for compatibility with Matlab and should\n\
+not be used unless you are porting old code that relies on this feature.\n\
+\n\
+To obtain short-circuit behavior for logical expressions in new programs,\n\
+you should always use the @samp{&&} and @samp{||} operators.\n\
address@hidden deftypefn")
+{
+  return SET_INTERNAL_VARIABLE (do_braindead_shortcircuit_evaluation);
+}
+
+DEFUN (foobar, , , "")
+{
+  tree_binary_expression x;
+  return octave_value (int (sizeof x));
+}
diff --git a/src/pt-exp.cc b/src/pt-exp.cc
--- a/src/pt-exp.cc
+++ b/src/pt-exp.cc
@@ -37,7 +37,7 @@
 // Expressions.
 
 bool
-tree_expression::is_logically_true (const char *warn_for)
+tree_expression::is_logically_true (const char *warn_for, bool)
 {
   bool expr_value = false;
 
diff --git a/src/pt-exp.h b/src/pt-exp.h
--- a/src/pt-exp.h
+++ b/src/pt-exp.h
@@ -69,7 +69,8 @@
 
   virtual bool is_boolean_expression (void) const { return false; }
 
-  virtual bool is_logically_true (const char *);
+  virtual bool is_logically_true (const char *warn_for,
+                                  bool braindead_shortcircuit = false);
 
   virtual bool lvalue_ok (void) const { return false; }
 
@@ -96,6 +97,8 @@
 
   virtual std::string original_text (void) const;
 
+  virtual void maybe_warn_braindead_shortcircuit (const std::string&) { }
+
   tree_expression *mark_in_parens (void)
     {
       num_parens++;