f(end) feature?

classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|

f(end) feature?

Paul Kienzle-2
octave:1> function y=f(x), y=x; end
octave:2> f(end)
ans = 1

Paul Kienzle
[hidden email]


Reply | Threaded
Open this post in threaded view
|

f(end) feature?

John W. Eaton-6
On 31-Dec-2002, Paul Kienzle <[hidden email]> wrote:

| octave:1> function y=f(x), y=x; end
| octave:2> f(end)
| ans = 1

Try the following patch.  With it, you should see

  octave:1> function y = f (x) y = x; end
  octave:2> f (end)
  error: invalid use of end
  error: evaluating argument list element number 1

jwe


2002-12-31  John W. Eaton  <[hidden email]>

        * pt-arg-list.cc (F__end__): Fail if rows or columns is negative.


Index: pt-arg-list.cc
===================================================================
RCS file: /usr/local/cvsroot/octave/src/pt-arg-list.cc,v
retrieving revision 1.12
diff -u -r1.12 pt-arg-list.cc
--- pt-arg-list.cc 21 Dec 2002 17:15:25 -0000 1.12
+++ pt-arg-list.cc 31 Dec 2002 20:15:07 -0000
@@ -104,7 +104,15 @@
  {
  case -1:
   // XXX FIXME XXX -- we really want "numel" here.
-  retval = indexed_object->rows () * indexed_object->columns ();
+  {
+    int nr = indexed_object->rows ();
+    int nc = indexed_object->columns ();
+
+    if (nr < 0 || nc < 0)
+      ::error ("invalid use of end");
+    else
+      retval = nr * nc;
+  }
   break;
 
  case 0:


Reply | Threaded
Open this post in threaded view
|

Re: f(end) feature?

Paul Kienzle-2
So I guess the following is out of the question:

        function y = head(x)
            y = x(1:min(3,end));
        end

- Paul

On Tue, Dec 31, 2002 at 02:16:14PM -0600, John W. Eaton wrote:

> On 31-Dec-2002, Paul Kienzle <[hidden email]> wrote:
>
> | octave:1> function y=f(x), y=x; end
> | octave:2> f(end)
> | ans = 1
>
> Try the following patch.  With it, you should see
>
>   octave:1> function y = f (x) y = x; end
>   octave:2> f (end)
>   error: invalid use of end
>   error: evaluating argument list element number 1
>
> jwe


Reply | Threaded
Open this post in threaded view
|

Re: f(end) feature?

John W. Eaton-6
On 31-Dec-2002, Paul Kienzle <[hidden email]> wrote:

| So I guess the following is out of the question:
|
| function y = head(x)
|    y = x(1:min(3,end));
| end

Sorry, that's not what I intended, and it's not actually prevented by
the previous patch, but min(3,end) would return -1, so you would not
get the result you expect.  The following patch should help (I think).

With it and the previous patch:

  octave:1> function y = head (x) y = x(1:min(3,end)); end
  octave:2> x = [1,2,3,4]
  x =

    1  2  3  4

  octave:3> head (x)
  ans =

    1  2  3

and

  octave:1> min (1,end)
  error: __end__: internal error
  error: evaluating argument list element number 2


Is that better?

The error message could be improved, I suppose.

jwe


2002-12-31  John W. Eaton  <[hidden email]>

        * pt-arg-list.cc (F__end__): Fail if rows or columns is negative.
        (tree_argument_list::convert_to_const_vector): Only protect and
        save pointer to the indexed object if it is a constant.


Index: pt-arg-list.cc
===================================================================
RCS file: /usr/local/cvsroot/octave/src/pt-arg-list.cc,v
retrieving revision 1.13
diff -u -r1.13 pt-arg-list.cc
--- pt-arg-list.cc 31 Dec 2002 20:16:38 -0000 1.13
+++ pt-arg-list.cc 31 Dec 2002 21:43:14 -0000
@@ -137,11 +151,19 @@
 octave_value_list
 tree_argument_list::convert_to_const_vector (const octave_value *object)
 {
-  unwind_protect::begin_frame ("convert_to_const_vector");
+  // END doesn't make sense for functions.  Maybe we need a different
+  // way of asking an octave_value object this question?
+
+  bool stash_object = (object && object->is_constant ());
 
-  unwind_protect_ptr (indexed_object);
+  if (stash_object)
+    {
+      unwind_protect::begin_frame ("convert_to_const_vector");
+
+      unwind_protect_ptr (indexed_object);
 
-  indexed_object = object;
+      indexed_object = object;
+    }
 
   int len = length ();
 
@@ -214,7 +236,8 @@
 
   args.resize (j);
 
-  unwind_protect::run_frame ("convert_to_const_vector");
+  if (stash_object)
+    unwind_protect::run_frame ("convert_to_const_vector");
 
   return args;
 }


Reply | Threaded
Open this post in threaded view
|

Re: f(end) feature?

Paul Kienzle-2
On Tue, Dec 31, 2002 at 03:47:02PM -0600, John W. Eaton wrote:

> On 31-Dec-2002, Paul Kienzle <[hidden email]> wrote:
>
> | So I guess the following is out of the question:
> |
> | function y = head(x)
> |    y = x(1:min(3,end));
> | end
>
> Sorry, that's not what I intended, and it's not actually prevented by
> the previous patch, but min(3,end) would return -1, so you would not
> get the result you expect.  The following patch should help (I think).
>
> With it and the previous patch:
>
>   octave:1> function y = head (x) y = x(1:min(3,end)); end
>   octave:2> x = [1,2,3,4]
>   x =
>
>     1  2  3  4
>
>   octave:3> head (x)
>   ans =
>
>     1  2  3
>
> and
>
>   octave:1> min (1,end)
>   error: __end__: internal error
>   error: evaluating argument list element number 2
>
>
> Is that better?

I'm impressed!  I still haven't figured out how you've done it.  Maybe
one day I'll give up and peek at the code.

- Paul


Reply | Threaded
Open this post in threaded view
|

Re: f(end) feature?

John W. Eaton-6
On 31-Dec-2002, Paul Kienzle <[hidden email]> wrote:

| I'm impressed!  I still haven't figured out how you've done it.  Maybe
| one day I'll give up and peek at the code.

The idea is simple, but maybe not obvious.

When Octave parses "end", it either recognizes it as a keyword (in
contexts like "for ... end") or an identifier (in argument list
contexts).  This works OK because a for loop or if conditional can't
appear inside an argument list (for value indexing or function calls).

If "end" is recognized as an identifier, it is converted to __end__,
to avoid (or create?)  some confusion.

There is also a new internal __end__ function that should be called
for any reference to __end__ (it is supposed to be impossible to clear
or redefine this function).  So we just need to arrange things so that
__end__ returns the proper value when it is called.

When you evaluate an expression like

  IDENTIFIER ( ARGS )

Octave creates an "index expression" object.  When it is evaluated, it
first converts ARGS to an array of values.  The function that does
that now takes a pointer to IDENTIFIER, so __end__ can ask that object
how many rows or columns it has.  To give __end__ a pointer to
IDENTIFIER, we use a global pointer and the unwind_protect stack (to
save and restore it reliably.  We also save the index position, so we
can tell __end__ which dimension of IDENTIFIER to return.

Then when __end__ is called, it checks the global pointer, and if it
is valid, asks the object that it points to what its size is and
returns the appropriate dimension.

Since we save and restore the global pointer with the unwind protect
stack, it should work correctly for recursion to any depth.

My second patch just ensured that we don't try to save a pointer to a
function object, that way if you have an expression like

  x (1, max (3, end))

then __end__ will have a pointer to x and not max when it is called,
and

  max (3, end)

by itself will not work (because end will not point to a valid
object).

jwe


Reply | Threaded
Open this post in threaded view
|

Re: f(end) feature?

Paul Kienzle-2
On Tue, Dec 31, 2002 at 04:31:48PM -0600, John W. Eaton wrote:
>
> Since we save and restore the global pointer with the unwind protect
> stack, it should work correctly for recursion to any depth.

So we pay for the feature even if we are not using it?  It's worth
it as far as I'm concerned, and perhaps even cheaper than doing
a call to a builtin function like length() or size().

The other option would be what? March up the parse tree when you
encounter an end in a subscript context and insert explicit save/restore
pointer commands for each identifier you encounter?  

- Paul


Reply | Threaded
Open this post in threaded view
|

Re: f(end) feature?

John W. Eaton-6
On 31-Dec-2002, Paul Kienzle <[hidden email]> wrote:

| On Tue, Dec 31, 2002 at 04:31:48PM -0600, John W. Eaton wrote:
| >
| > Since we save and restore the global pointer with the unwind protect
| > stack, it should work correctly for recursion to any depth.
|
| So we pay for the feature even if we are not using it?  It's worth
| it as far as I'm concerned, and perhaps even cheaper than doing
| a call to a builtin function like length() or size().

Yes, but at least it works.  The cost is:

  Pass a pointer in each call to tree_argument_list::convert_to_const_vector
  even if it is not needed.

  In convert_to_const_vector, do

    bool stash_object = (object && object->is_constant ());

    if (stash_object)
      {
        unwind_protect::begin_frame ("convert_to_const_vector");

        unwind_protect_ptr (indexed_object);

        indexed_object = object;
      }

  at the beginning of the function,

    index_position = (len == 1) ? -1 : k;

  for each element of the argument list, and then

    if (stash_object)
      unwind_protect::run_frame ("convert_to_const_vector");

  at the end of the function.

Most of the overhead is in handling the unwind_protect stack.  To do
better, we could try to mark expressions that might actually need to
have the global variables set, then only do the save/restore for those
expressions that have been marked.  This would be similar to your idea
of modifying the parse tree to insert some code where needed.  I'll
see whether that would be easy to do.

jwe


Reply | Threaded
Open this post in threaded view
|

Re: f(end) feature?

John W. Eaton-6
On 31-Dec-2002, John W. Eaton <[hidden email]> wrote:

| On 31-Dec-2002, Paul Kienzle <[hidden email]> wrote:
|
| | On Tue, Dec 31, 2002 at 04:31:48PM -0600, John W. Eaton wrote:
| | >
| | > Since we save and restore the global pointer with the unwind protect
| | > stack, it should work correctly for recursion to any depth.
| |
| | So we pay for the feature even if we are not using it?  It's worth
| | it as far as I'm concerned, and perhaps even cheaper than doing
| | a call to a builtin function like length() or size().
|
| Yes, but at least it works.  The cost is:
|
|   Pass a pointer in each call to tree_argument_list::convert_to_const_vector
|   even if it is not needed.
|
|   In convert_to_const_vector, do
|
|     bool stash_object = (object && object->is_constant ());
|
|     if (stash_object)
|       {
| unwind_protect::begin_frame ("convert_to_const_vector");
|
| unwind_protect_ptr (indexed_object);
|
| indexed_object = object;
|       }
|
|   at the beginning of the function,
|
|     index_position = (len == 1) ? -1 : k;
|
|   for each element of the argument list, and then
|
|     if (stash_object)
|       unwind_protect::run_frame ("convert_to_const_vector");
|
|   at the end of the function.
|
| Most of the overhead is in handling the unwind_protect stack.  To do
| better, we could try to mark expressions that might actually need to
| have the global variables set, then only do the save/restore for those
| expressions that have been marked.  This would be similar to your idea
| of modifying the parse tree to insert some code where needed.  I'll
| see whether that would be easy to do.

OK, it was not hard.  Here is the patch.

jwe


src/ChangeLog:

2003-01-01  John W. Eaton  <[hidden email]>

        * pt-arg-list.cc (tree_argument_list::append): New function.
        (tree_argument_list::convert_to_const_vector): Don't save and
        set pointer to indexed object if list_includes_magic_end is false.
        * pt-arg-list.h (tree_argument_list::append): Provide decl.
        (tree_argument_list::list_includes_magic_end): New data member.
        (tree_argument_list::tree_argument_list): Initialize it.


Index: pt-arg-list.h
===================================================================
RCS file: /usr/local/cvsroot/octave/src/pt-arg-list.h,v
retrieving revision 1.9
diff -u -r1.9 pt-arg-list.h
--- pt-arg-list.h 21 Dec 2002 17:15:25 -0000 1.9
+++ pt-arg-list.h 2 Jan 2003 03:32:13 -0000
@@ -45,9 +45,13 @@
 {
 public:
 
-  tree_argument_list (void) { }
+  typedef tree_expression* element_type;
 
-  tree_argument_list (tree_expression *t) { append (t); }
+  tree_argument_list (void)
+    : list_includes_magic_end (false) { }
+
+  tree_argument_list (tree_expression *t)
+    : list_includes_magic_end (false) { append (t); }
 
   ~tree_argument_list (void);
 
@@ -59,6 +63,8 @@
       return retval;
     }
 
+  void append (const element_type& s);
+
   int nargout_count (void) const;
 
   bool all_elements_are_constant (void) const;
@@ -70,6 +76,8 @@
   void accept (tree_walker& tw);
 
 private:
+
+  bool list_includes_magic_end;
 
   // No copying!

Index: pt-arg-list.cc
===================================================================
RCS file: /usr/local/cvsroot/octave/src/pt-arg-list.cc,v
retrieving revision 1.14
diff -u -r1.14 pt-arg-list.cc
--- pt-arg-list.cc 31 Dec 2002 21:48:54 -0000 1.14
+++ pt-arg-list.cc 2 Jan 2003 03:32:13 -0000
@@ -57,6 +57,15 @@
     }
 }
 
+void
+tree_argument_list::append (const element_type& s)
+{
+  octave_base_list<tree_expression *>::append (s);
+
+  if (s && s->is_identifier () && s->name () == "__end__")
+    list_includes_magic_end = true;
+}
+
 int
 tree_argument_list::nargout_count (void) const
 {
@@ -154,7 +163,8 @@
   // END doesn't make sense for functions.  Maybe we need a different
   // way of asking an octave_value object this question?
 
-  bool stash_object = (object && object->is_constant ());
+  bool stash_object = (list_includes_magic_end
+       && object && object->is_constant ());
 
   if (stash_object)
     {


Reply | Threaded
Open this post in threaded view
|

Re: f(end) feature?

John W. Eaton-6
On  1-Jan-2003, John W. Eaton <[hidden email]> wrote:

| OK, it was not hard.  Here is the patch.

Oops.  It is not that easy.  My code did this:

  void
  tree_argument_list::append (const element_type& s)
  {
    octave_base_list<tree_expression *>::append (s);

    if (s && s->is_identifier () && s->name () == "__end__")
      list_includes_magic_end = true;
  }

but (obviously, now) it needs to check the possibility that any
expression type might also contain a magic end token.  Should not be
too hard, but will require some new code in all the expression types.
Will try to do tomorrow, and make a new snapshot within a day or two.
In the meantime, if you want "end" to work, you can comment out the if
statement in this function (but leave the assignment of true to
list_includes_magic_end)

jwe