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

[Jaroslav Hajek]

On Thu, Oct 7, 2010 at 7:42 PM, John W. Eaton <address@hidden> wrote:
> On  7-Oct-2010, Jaroslav Hajek wrote:
>
> | I think the case is analogous to the non-integer ranges, so the
> | general idea is OK. What I see as a downside of this solution is that
> | *each* binop node gets an extra flag, which will make all expression
> | trees consume more space just because of a stupid feature.
>
> It's just a single bool.  Currently we have the folowing data members
> in the tree_binary_expression hierarchy:
>
>  class tree:
>    2 ints
>    1 bool
>
>  class tree_expression, derived from tree:
>    1 int
>    2 bool
>
>  class tree_binary_expression, drived from tree_expression:
>    2 pointers
>    1 enum
>
> for a total of 2 pointers, 3 ints, 3 bools and 1 enum (without the
> proposed change; add 1 bool with it).
>
> On my system, the overall size is 56 bytes without the change and 56
> bytes with it.  So there is no change, apparently due to padding.
> This result might be diifferent for 32 bit systems.  I haven't checked.
>
> | Here's an alternative idea: Maybe it would be be better to make
> | tree_expression::is_logically_true accept an optional short_circuit
> | flag and then override this method directly in tree_binary_expression
> | to do the right thing?
> | It would then suffice to call
> | expr->is_logically_true ("if", Vdo_braindead_shortcircuit_evaluation)
> | from within pt-eval.cc at all appropriate places (also in while,
>
> The definition of tree_expression::is_logically_true is:
>
>  bool
>  tree_expression::is_logically_true (const char *warn_for)
>  {
>    bool expr_value = false;
>
>    octave_value t1 = rvalue1 ();
>
>    if (! error_state)
>      {
>        if (t1.is_defined ())
>          return t1.is_true ();
>        else
>          ::error ("%s: undefined value used in conditional expression",
>                   warn_for);
>      }
>
>    return expr_value;
>  }
>
> Since in expressions like
>
>  if (a | (b & c))
>
> both operators are subject to short-circuit behavior, we would the
> have to pass the flag on to all rvalue1 functions.  It seems like a
> lot of code would have to change to accomodate this approach and the
> rvalue1 function already accepts one optional parameter.  Or do you
> see another way?


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;
}

when or how would that cause trouble? the if-handler would just call
this with the flag on and it would be passed to both subexpressions.
When an expression is not | or &, it just calls the inherited handler
that ignores the flag thus it doesn't get passed to any
subexpressions. It seems to me that this would work for all your
examples. If I am wrong, please explain.

> I understand the motivation to avoid paying a
> penalty for something that is rarely used, but given that change in
> the size of the object will likely be small (or zero) relative to the
> current size, and that changing is_logically_true apparently requires
> changing calls to rvalue1 nearly everywhere, it seems simpler to add
> the field to the tree_binary_expression object.
>

Yeah, you've shown that the penalty is negligible anyway, so it
doesn't really matter. A practical lesson :) Still, unless my solution
is broken, it seems less intrusive to me.

> BTW, note that in expressions like
>
>  if (x (a | b))
>
> or even
>
>  if (x || (a & b))
>
> the & and | operators do not short-circuit.
>
> | perhaps in until for consistency?)
>
> Since I'm adding this feature only to make it possible to run Matlab
> code that relies on the behavior andf Matlab does not have do-until,
> I'd prefer to not do that.  People porting code should never need this
> behavior in do-until, and people writing new code should never use it.
>

OK

regards