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

[John W. Eaton]

I'm considering applying the following patch so that Octave can be
compatible with the stupid Matlab-style short-circuit behavior for the
normally element-by-element | and & operators.

No, I have not (entirely) lost my mind.  I made this change in
response to a request from someone who is paying for support.  I still
think this is a really really stupid feature, but I don't see the harm
if it is off by default and only enabled in --traditional AKA
--maximum-braindamage mode, or if you explicitly set the
do_braindead_shortcircuit_evaluation pseudo-variable to true.  The
help text for that function specifically warns against using this
feature in new code and recommends the || and && operators.

Since it only affects the way that expressions are evaluated and not
the way they are parsed, the feature can be toggled at run time.  For
example:

  octave> function r = z (a), disp ('in z'); r = a; end
  octave> function f (a), if (a | z (0)) end, end
  octave> do_braindead_shortcircuit_evaluation (1)  ## short-circuit
  octave> f(1)
  octave> do_braindead_shortcircuit_evaluation (0)  ## no short-circuit
  octave> f (1)
  in z

I'm definitely not encouraging the use of this feature.  We should
probably ensure that there are no | or & expressions in IF or WHILE
statements within Octave itself that could be misinterpreted if this
feature is enabled.  Perhaps there should also be a warning (issued
by the parser) about expressions that are marked as eligible for this
kind of short-circuit evaluation?  That would make finding them
slightly easier.

Are there any strong objections to making this change?

jwe

# HG changeset patch
# User John W. Eaton <address@hidden>
# Date 1286444672 14400
# Node ID 235cdbfa204392ffc00dffbf2264fff671f4551e
# Parent  0f6c5efce96e590455b45df646af5cc7af80ab96
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,23 @@
+2010-10-07  John W. Eaton  <address@hidden>
+
+       * octave.cc (maximum_braindamage): Set
+       do_braindead_shortcircuit_evaluation to true.
+       * oct-parse.yy (if_cmd_list1, elseif_clause, loop_command):
+       Mark conditions in IF and WHILE statements for braindead
+       short-circuit behavior.
+       * pt-binop.cc (Vdo_braindead_shortcircuit_evaluation): New
+       static variable.
+       (Fdo_braindead_shortcircuit_evaluation): New function.
+       (tree_binary_expression::rvalue1): Perform short-circuit
+       evaluation of | and & expressions that are conditions of WHILE
+       and IF statements if Vdo_braindead_shortcircuit_evaluation is true.
+       * pt-binop.h
+       (tree_binary_expression::eligible_for_braindead_shortcircuit):
+       New data member.  Initialize it in class constructors.
+       (tree_binary_expression::mark_braindead_shortcircuit): New function.
+       * pt-exp.h (tree_expression::mark_braindead_shortcircuit):
+       New virtual function.
+
 2010-10-07  John W. Eaton  <address@hidden>
 
        * DLD-FUNCTIONS/conv2.cc (convn): Style fixes.  Edit docstring.
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->mark_braindead_shortcircuit ();
+
+                    $$ = 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->mark_braindead_shortcircuit ();
+
+                    $$ = 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->mark_braindead_shortcircuit ();
+
                     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
@@ -26,11 +26,17 @@
 #endif
 
 #include "error.h"
+#include "defun.h"
 #include "oct-obj.h"
 #include "ov.h"
 #include "pt-binop.h"
 #include "pt-bp.h"
 #include "pt-walk.h"
+#include "variables.h"
+
+// TRUE means we mark | and & expressions for braindead short-circuit
+// behavior.
+static bool Vdo_braindead_shortcircuit_evaluation;
 
 // Binary expressions.
  
@@ -56,6 +62,55 @@
   if (error_state)
     return retval;
 
+  if (Vdo_braindead_shortcircuit_evaluation
+      && eligible_for_braindead_shortcircuit)
+    {
+      if (op_lhs)
+        {
+          octave_value a = op_lhs->rvalue1 ();
+
+          if (! error_state)
+            {
+              if (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);
+                    }
+                }
+            }
+        }
+    }
+
   if (op_lhs)
     {
       octave_value a = op_lhs->rvalue1 ();
@@ -207,3 +262,21 @@
 
   return new_be;
 }
+
+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);
+}
diff --git a/src/pt-binop.h b/src/pt-binop.h
--- a/src/pt-binop.h
+++ b/src/pt-binop.h
@@ -46,13 +46,15 @@
   tree_binary_expression (int l = -1, int c = -1,
                           octave_value::binary_op t
                             = octave_value::unknown_binary_op)
-    : tree_expression (l, c), op_lhs (0), op_rhs (0), etype (t) { }
+    : tree_expression (l, c), op_lhs (0), op_rhs (0), etype (t),
+      eligible_for_braindead_shortcircuit (false) { }
 
   tree_binary_expression (tree_expression *a, tree_expression *b,
                           int l = -1, int c = -1,
                           octave_value::binary_op t
                             = octave_value::unknown_binary_op)
-    : tree_expression (l, c), op_lhs (a), op_rhs (b), etype (t) { }
+    : tree_expression (l, c), op_lhs (a), op_rhs (b), etype (t),
+      eligible_for_braindead_shortcircuit (false) { }
 
   ~tree_binary_expression (void)
     {
@@ -60,6 +62,18 @@
       delete op_rhs;
     }
 
+  void mark_braindead_shortcircuit (void)
+    {
+      if (etype == octave_value::op_el_and
+          || etype == octave_value::op_el_or)
+        {
+          eligible_for_braindead_shortcircuit = true;
+
+          op_lhs->mark_braindead_shortcircuit ();
+          op_rhs->mark_braindead_shortcircuit ();
+        }
+    }
+
   bool has_magic_end (void) const
     {
       return ((op_lhs && op_lhs->has_magic_end ())
@@ -97,6 +111,10 @@
   // The type of the expression.
   octave_value::binary_op etype;
 
+  // TRUE if this is an | or & expression in the condition of an IF
+  // or WHILE statement.
+  bool eligible_for_braindead_shortcircuit;
+
   // No copying!
 
   tree_binary_expression (const tree_binary_expression&);
diff --git a/src/pt-exp.h b/src/pt-exp.h
--- a/src/pt-exp.h
+++ b/src/pt-exp.h
@@ -96,6 +96,8 @@
 
   virtual std::string original_text (void) const;
 
+  virtual void mark_braindead_shortcircuit (void) { }
+
   tree_expression *mark_in_parens (void)
     {
       num_parens++;