Skip to content

Commit

Permalink
avoid error with ignored values returned from function calls (bug #65…
Browse files Browse the repository at this point in the history
…153)

* pt-eval.cc, pt-eval.h
(tree_evaluator::convert_return_list_to_const_vector):
Eliminate ignored_outputs argument.  Don't consider ignored outputs
when constructing list of values from the list of parameters returned
from a function.  Change all callers.

* test/bug-65153.tst: New file.
* test/Makefile.am: Update.
  • Loading branch information
jweaton committed Jan 19, 2024
1 parent f98ef8d commit 7dd7521
Show file tree
Hide file tree
Showing 4 changed files with 87 additions and 26 deletions.
34 changes: 10 additions & 24 deletions libinterp/parse-tree/pt-eval.cc
Original file line number Diff line number Diff line change
Expand Up @@ -2331,8 +2331,7 @@ tree_evaluator::convert_to_const_vector (tree_argument_list *args)

octave_value_list
tree_evaluator::convert_return_list_to_const_vector
(tree_parameter_list *ret_list, int nargout, const Matrix& ignored_outputs,
const Cell& varargout)
(tree_parameter_list *ret_list, int nargout, const Cell& varargout)
{
octave_idx_type vlen = varargout.numel ();
int len = ret_list->length ();
Expand All @@ -2343,9 +2342,6 @@ tree_evaluator::convert_return_list_to_const_vector
else
{
int i = 0;
int k = 0;
int num_ignored = ignored_outputs.numel ();
int ignored = num_ignored > 0 ? ignored_outputs(k) - 1 : -1;

if (nargout <= len)
{
Expand All @@ -2357,14 +2353,10 @@ tree_evaluator::convert_return_list_to_const_vector
if (nargout == 0 && ! is_defined (elt->ident ()))
break;

if (ignored >= 0 && i == ignored)
{
i++;
k++;
ignored = k < num_ignored ? ignored_outputs(k) - 1 : -1;
}
else
retval(i++) = evaluate (elt);
if (is_defined (elt->ident ()))
retval(i) = evaluate (elt);

i++;

if (i == nout)
break;
Expand All @@ -2378,14 +2370,10 @@ tree_evaluator::convert_return_list_to_const_vector

for (tree_decl_elt *elt : *ret_list)
{
if (ignored >= 0 && i == ignored)
{
i++;
k++;
ignored = k < num_ignored ? ignored_outputs(k) - 1 : -1;
}
else
retval(i++) = evaluate (elt);
if (is_defined (elt->ident ()))
retval(i) = evaluate (elt);

i++;
}

for (octave_idx_type j = 0; j < vlen; j++)
Expand Down Expand Up @@ -3710,9 +3698,7 @@ tree_evaluator::execute_user_function (octave_user_function& user_function,
varargout = varargout_varval.xcell_value ("varargout must be a cell array object");
}

retval = convert_return_list_to_const_vector (ret_list, nargout,
ignored_outputs,
varargout);
retval = convert_return_list_to_const_vector (ret_list, nargout, varargout);
}

return retval;
Expand Down
3 changes: 1 addition & 2 deletions libinterp/parse-tree/pt-eval.h
Original file line number Diff line number Diff line change
Expand Up @@ -410,8 +410,7 @@ class OCTINTERP_API tree_evaluator : public tree_walker

octave_value_list
convert_return_list_to_const_vector
(tree_parameter_list *ret_list, int nargout,
const Matrix& ignored_outputs, const Cell& varargout);
(tree_parameter_list *ret_list, int nargout, const Cell& varargout);

bool eval_decl_elt (tree_decl_elt *elt);

Expand Down
1 change: 1 addition & 0 deletions test/Makefile.am
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ TEST_FILES += \
bug-55322.tst \
bug-59950.tst \
bug-61201.tst \
bug-65153.tst \
colormaps.tst \
command.tst \
complex.tst \
Expand Down
75 changes: 75 additions & 0 deletions test/bug-65153.tst
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
%!function retval = bug65153_flipud (x)
%! global bug65153_global_isargout
%! bug65153_global_isargout = isargout (1);
%! retval = flipud (x);
%!endfunction

## Bug 65153 revealed two problems related to the way Octave was
## attempting to handle ignored function outputs. One is that an
## ignored output can propagate from one function to another in some
## contexts. For example, evaluating
##
## [~, y] = bug65153_1 ()
##
## results in isargout(1) -> false in the call to bug65153_flipud.
## The second problem is that even though the return value is always
## set in bug65153_flipud, Octave was using the equivalent of the
## isargout information internally and not assigning the return value
## internally, so the matrix construction failed. The second problem
## is easier to solve than the first. We test for each problem
## separately here.

%!function [x, y] = bug65153_1 ()
%! n = 10;
%! x = (1:n)';
%! y = (1:n)';
%! [(1:n)', bug65153_flipud((1:n)')];
%!endfunction

## The bug65153_2 function tests the same problem as bug65153_1 but for
## a different context. ANS should be assigned after the call to
## bug65153_flipud but it was not because of Octave's incorrect internal
## handling of ignored outputs.

%!function x = bug65153_2 ()
%! global bug65153_global
%! bug65153_flipud ((1:10)');
%! x = ans;
%! bug65153_global = x;
%!endfunction

%!test <65153>
%! global bug65153_global bug65153_global_isargout
%! unwind_protect
%! [~, y] = bug65153_1 ();
%! assert (y, (1:10)');
%! unwind_protect_cleanup
%! clear -global bug65153_global bug65153_global_isargout
%! end_unwind_protect

%!test <65153>
%! global bug65153_global bug65153_global_isargout
%! unwind_protect
%! [~, y] = bug65153_1 ();
%! assert (bug65153_global_isargout, true);
%! unwind_protect_cleanup
%! clear -global bug65153_global bug65153_global_isargout
%! end_unwind_protect

%!test <65153>
%! global bug65153_global bug65153_global_isargout
%! unwind_protect
%! [~] = bug65153_2 ();
%! assert (bug65153_global, flipud ((1:10)'));
%! unwind_protect_cleanup
%! clear -global bug65153_global bug65153_global_isargout
%! end_unwind_protect

%!test <65153>
%! global bug65153_global bug65153_global_isargout
%! unwind_protect
%! [~] = bug65153_2 ();
%! assert (bug65153_global_isargout, true);
%! unwind_protect_cleanup
%! clear -global bug65153_global bug65153_global_isargout
%! end_unwind_protect

0 comments on commit 7dd7521

Please sign in to comment.