bugGNU Octave - Bugs: bug #54465, Short Circuit Operators not...

 
 

bug #54465: Short Circuit Operators not working with negation operator

Submitter:  Andrew Lofts <andrewlofts>
Submitted:  Wed 08 Aug 2018 06:19:04 PM UTC
   
 
Category:  Interpreter Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Matlab Compatibility
Status:  Fixed Assigned to:  None
Originator Name:  AndrewLofts Open/Closed:  * Closed
Release:  * 4.4.1-rc2 Operating System:  * GNU/Linux
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Tue 28 Aug 2018 11:42:52 PM UTC, comment #10: 

I commented out two functions to suppress the compilation warnings in this cset (https://hg.savannah.gnu.org/hgweb/octave/rev/d0062829754a).

Rik <rik5>
Group administrator
Tue 28 Aug 2018 08:36:20 PM UTC, comment #9: 

Yeah, I eliminated the only call to that function.  We can just comment it out for now.

John W. Eaton <jwe>
Group administrator
Tue 28 Aug 2018 07:55:59 PM UTC, comment #8: 

The change seems to work.  I used


cd test
test parser.tst


and all 52 tests pass.

I get this warning during compilation though:


 CXX      libinterp/parse-tree/libinterp_parse_tree_libparse_tree_la-oct-parse.lo
libinterp/parse-tree/pt-cbinop.cc:130:3: warning: ‘octave_value::compound_binary_op octave::simplify_and_or_op(octave::tree_expression*&, octave::tree_expression*&, octave_value::binary_op)’ defined but not used [-Wunused-function]
   simplify_and_or_op (tree_expression_ptr_t& a, tree_expression_ptr_t& b,
   ^




Rik <rik5>
Group administrator
Tue 28 Aug 2018 06:03:37 PM UTC, comment #7: 

How about the following change?

http://hg.savannah.gnu.org/hgweb/octave/rev/8ac4bfa55053

With more work, maybe the optimization for the compound negation and elementwise logical operators could be preserved when the expression is not eligible for short circuiting.

John W. Eaton <jwe>
Group administrator
Tue 28 Aug 2018 03:32:16 PM UTC, comment #6: 

@jwe: This patch seems to have introduced a regression in performance which is documented at bug #45890.

Is there a way to be more clever about this so that certain compound expressions like


A * B'


can be recognized as a single operation (pass multiply with appropriate flag to BLAS) rather than two operations (multiply done by BLAS, transpose done by Octave)?

Rik <rik5>
Group administrator
Thu 09 Aug 2018 04:47:43 PM UTC, comment #5: 

I see that jwe just pushed the patch for this to the stable branch.  Marking as fixed and closing report.

Rik <rik5>
Group administrator
Wed 08 Aug 2018 08:46:36 PM UTC, comment #4: 

The patch seems to work.  Here are two tests


%!test
%! x = 0;
%! if (! 0 || (x = 1))
%! endif
%! assert (x == 0);

%!test
%! x = 0;
%! if (! 0 | (x = 1))
%! endif
%! assert (x == 0);


Also attached as a file.  They should probably go in test/parser.tst and have a bug number associated.


(file #44743)

Rik <rik5>
Group administrator
Wed 08 Aug 2018 08:23:03 PM UTC, comment #3: 

I think the attached patch should fix the problem.  But before committing, it needs a test and we should probably see whether it makes sense to try to preserve the compound binary operator optimization or simply remove it.


(file #44742)

John W. Eaton <jwe>
Group administrator
Wed 08 Aug 2018 07:20:56 PM UTC, comment #2: 

This problem appears to be some kind of incorrect handling of precedence.  The evaluator is evaluating the "b == 2" expression first.

In any case, I recommend using || and && if you want short-circuit evaluation.

John W. Eaton <jwe>
Group administrator
Wed 08 Aug 2018 07:01:54 PM UTC, comment #1: 

Confirmed.  Incidentally, version 4.2.1 is very old and no longer supported.  You will have a better experience if you can update to the latest stable version.  But, I confirmed this on the current development code as well.

Here is an even more concise example:


clear b;

if ((! 0) || b == 2)
  disp ('Octave short-circuit');
end

if ((! 0) | b == 2)
  disp ('Matlab short-circuit');
end


I used Octave's own negation operator just to make sure there wasn't something funny going on.  I also removed the call to exist() and just use a constant.  Octave's own short-circuiting operator works in this case, so this is an error in the parser because within if statements the '|' operator should map to '||' for logical statements.

Adding jwe to the CC list since he has spent the most time with this code. 



(file #44741)

Rik <rik5>
Group administrator
Wed 08 Aug 2018 06:19:04 PM UTC, original submission:  

Hello, I am working on porting MATLAB to Octave, normally | and & will short circuit in if() statements just like MATLAB. However if a function with a negative is used like ~exist(),~strcmp() or ~isempty(), the statements do not seam to short circuit like MATLABs do. ~= and the functions without ~, however do work. Example: (Octave/Matlab)


%From an empty workspace
%Octave
if ~exist('a','var') | b == 2
     disp('TRUE');
end
error: 'b' undefined near line 1 column 27

%Matlab
if ~exist('a','var') | b == 2
     disp('TRUE');
end
TRUE


Setting do_braindead_shortcircuit_evaluation doesn't seam to change anything.
Thank you for your help,
Andrew

Andrew Lofts <andrewlofts>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files
file #44743:  tst_sc2.m added by rik5 (138B - text/x-matlab)
file #44742:  diffs.txt added by jwe (447B - text/plain)
file #44741:  tst_sc.m added by rik5 (127B - text/x-matlab)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by rik5
  • -email is unavailable- added by rik5 (Updated the item)
  • -email is unavailable- added by andrewlofts (Submitted the item)
  • -email is unavailable- added by andrewlofts (Octave,Matlab ~SC)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only group members can vote.

     

    Follow 13 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2018-08-09 rik5 StatusPatch Reviewed Fixed
        Open/ClosedOpen Closed
        Releasedev 4.4.1-rc2
    2018-08-08 rik5 Attached File- Added tst_sc2.m, #44743
        CategoryNone Interpreter
        StatusConfirmed Patch Reviewed
    2018-08-08 jwe Attached File- Added diffs.txt, #44742
    2018-08-08 rik5 Carbon-Copy- Added jwe
    2018-08-08 rik5 Attached File- Added tst_sc.m, #44741
        StatusNone Confirmed
        Release4.2.1 dev
        SummaryShort Circuit Operators not working for ~functions Short Circuit Operators not working with negation operator
    2018-08-08 andrewlofts Carbon-Copy- Added andrewlofts

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code