[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: stupid Matlab-style short-circuit behavior for | and &
From: |
John W. Eaton |
Subject: |
Re: stupid Matlab-style short-circuit behavior for | and & |
Date: |
Thu, 7 Oct 2010 16:39:23 -0400 |
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++;
- stupid Matlab-style short-circuit behavior for | and &, John W. Eaton, 2010/10/07
- Re: stupid Matlab-style short-circuit behavior for | and &, Søren Hauberg, 2010/10/07
- Re: stupid Matlab-style short-circuit behavior for | and &, CdeMills, 2010/10/07
- Re: stupid Matlab-style short-circuit behavior for | and &, Jaroslav Hajek, 2010/10/07
- Re: stupid Matlab-style short-circuit behavior for | and &, John W. Eaton, 2010/10/07
- Re: stupid Matlab-style short-circuit behavior for | and &, Jaroslav Hajek, 2010/10/07
- Re: stupid Matlab-style short-circuit behavior for | and &,
John W. Eaton <=
- Re: stupid Matlab-style short-circuit behavior for | and &, Jaroslav Hajek, 2010/10/08
- Re: stupid Matlab-style short-circuit behavior for | and &, John W. Eaton, 2010/10/08
- Re: stupid Matlab-style short-circuit behavior for | and &, Søren Hauberg, 2010/10/09
- Re: stupid Matlab-style short-circuit behavior for | and &, Ben Abbott, 2010/10/09
- Re: stupid Matlab-style short-circuit behavior for | and &, John W. Eaton, 2010/10/09
- Re: stupid Matlab-style short-circuit behavior for | and &, Jaroslav Hajek, 2010/10/09
- Re: stupid Matlab-style short-circuit behavior for | and &, Søren Hauberg, 2010/10/09