fn().field broken?

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

fn().field broken?

Paul Kienzle-2
Hi,

I've just ran through octave-forge/batch_test on my oct-file enhanced
cygwin build. A couple of things are broken as expected since some of the
octave internals have changed.

I am surprised that
        stat("test.jpg").size
returns the entire structure instead of just the size element.  Is this
behaviour expected?

I will try things again under linux tomorrow.

Paul Kienzle
[hidden email]


Reply | Threaded
Open this post in threaded view
|

fn().field broken?

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

| I am surprised that
| stat("test.jpg").size
| returns the entire structure instead of just the size element.  Is this
| behaviour expected?

No, it looks like a bug.  I'll try to fix this soon.

Thanks,

jwe


Reply | Threaded
Open this post in threaded view
|

fn().field broken?

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

| On  8-Sep-2002, Paul Kienzle <[hidden email]> wrote:
|
| | I am surprised that
| | stat("test.jpg").size
| | returns the entire structure instead of just the size element.  Is this
| | behaviour expected?
|
| No, it looks like a bug.  I'll try to fix this soon.

I've checked in the following patch.

Note the comment I added:

  // XXX FIXME XXX -- Note that if a function call returns multiple
  // values, and there is further indexing to perform, then we are
  // ignoring all but the first value.  Is this really what we want to
  // do?  If it is not, then what should happen for stat("file").size,
  // for exmaple?

Currently, some functions (like stat) return multiple values even when
nargout is only 0 or 1, and then the extra values are simply ignored.
I think it's easier to implement some of these functions if that's the
way things work.  I suppose we could tighten up the error checking
here if we required functions to only return as many values as
requested, but that seems like a lot of extra work to do.  Or, we
could only allow further indexing when nargout is 0 or 1.  What do you
think?

Thanks,

jwe


Index: src/ov-builtin.cc
===================================================================
RCS file: /usr/local/cvsroot/octave/src/ov-builtin.cc,v
retrieving revision 1.5
diff -u -r1.5 ov-builtin.cc
--- src/ov-builtin.cc 15 May 2002 03:21:00 -0000 1.5
+++ src/ov-builtin.cc 9 Sep 2002 18:29:44 -0000
@@ -76,10 +76,20 @@
       panic_impossible ();
     }
 
-  return retval;
+  // XXX FIXME XXX -- perhaps there should be an
+  // octave_value_list::next_subsref member function?  See also
+  // octave_user_function::subsref.
+  //
+  // XXX FIXME XXX -- Note that if a function call returns multiple
+  // values, and there is further indexing to perform, then we are
+  // ignoring all but the first value.  Is this really what we want to
+  // do?  If it is not, then what should happen for stat("file").size,
+  // for exmaple?
+
+  if (idx.length () > 1)
+    retval = retval(0).next_subsref (type, idx);
 
-  // XXX FIXME XXX
-  //  return retval.next_subsref (type, idx);
+  return retval;
 }
 
 octave_value_list
Index: src/ov-usr-fcn.cc
===================================================================
RCS file: /usr/local/cvsroot/octave/src/ov-usr-fcn.cc,v
retrieving revision 1.29
diff -u -r1.29 ov-usr-fcn.cc
--- src/ov-usr-fcn.cc 4 Jul 2002 17:43:00 -0000 1.29
+++ src/ov-usr-fcn.cc 9 Sep 2002 18:29:45 -0000
@@ -328,10 +328,14 @@
       panic_impossible ();
     }
 
-  return retval;
+  // XXX FIXME XXX -- perhaps there should be an
+  // octave_value_list::next_subsref member function?  See also
+  // octave_builtin::subsref.
+
+  if (idx.length () > 1)
+    retval = retval(0).next_subsref (type, idx);
 
-  // XXX FIXME XXX
-  //  return retval.next_subsref (type, idx);
+  return retval;
 }
 
 octave_value_list


Reply | Threaded
Open this post in threaded view
|

Re: fn().field broken?

Paul Kienzle-2
On Mon, Sep 09, 2002 at 01:40:45PM -0500, John W. Eaton wrote:

> On  8-Sep-2002, John W. Eaton <[hidden email]> wrote:
>
> | On  8-Sep-2002, Paul Kienzle <[hidden email]> wrote:
> |
> | | I am surprised that
> | | stat("test.jpg").size
> | | returns the entire structure instead of just the size element.  Is this
> | | behaviour expected?
> |
> | No, it looks like a bug.  I'll try to fix this soon.
>
> I've checked in the following patch.
>
> Note the comment I added:
>
>   // XXX FIXME XXX -- Note that if a function call returns multiple
>   // values, and there is further indexing to perform, then we are
>   // ignoring all but the first value.  Is this really what we want to
>   // do?  If it is not, then what should happen for stat("file").size,
>   // for exmaple?
>
> Currently, some functions (like stat) return multiple values even when
> nargout is only 0 or 1, and then the extra values are simply ignored.
> I think it's easier to implement some of these functions if that's the
> way things work.  I suppose we could tighten up the error checking
> here if we required functions to only return as many values as
> requested, but that seems like a lot of extra work to do.  Or, we
> could only allow further indexing when nargout is 0 or 1.  What do you
> think?

The only place I force 2 output arguments is in the filter design functions
since a filter with only A and not B doesn't make any sense.  Within
the function I check for nargout == 2, or generate an error.  

Since the need to do otherwise is rare and since you can force it if you
need it by generating your own errors, I think the interpreter should use
only as many return values as it needs, at least in scripts.

For compiled functions, I would prefer having the interpreter allocate the
slots it needs for the return values and pass a handle for it to the
interpreted function.  I noticed in profiling that a big chunk of time is
spent in class constructors and destructors.  Modifying handles rather than
returning objects from functions should reduce this overhead.  Returning
pointers to octave values rather than copying octave values to an array
of octave_values would reduce overhead even further.

Try the following using the attached churn.cc:

octave:1> tic; churn; toc
ans = 0.15586
octave:2> tic; churn; toc
ans = 0.11828
octave:3> tic; nochurn; toc
ans = 0.012887
octave:4> tic; noovchurn; toc
ans = 0.0094780

I'm guessing these changes are too big to go in before 2.2.

- Paul

---- churn.cc -----

#include <octave/oct.h>

octave_value_list churn(int n)
{
  octave_value_list ret;

  ret(0) = octave_value(1.0);
  return ret;
}

DEFUN_DLD(churn,args,,"test octave_value_list return")
{
  for (int i=1; i < 10000; i++)
    {
      octave_value_list ret = churn(1);
    }

  return octave_value_list();
}

int nochurn(int n, octave_value ret[])
{
  if (n > 0)
    {
      ret[0] = octave_value(1.0);
      return 1;
    }
  else
    return 0;
}

DEFUN_DLD(nochurn,args,,"test array of octave values")
{
  for (int i=1; i < 10000; i++)
    {
      octave_value ret[1];
      int rets = nochurn(1,ret);
    }
  return octave_value_list();
}

int noovchurn(int n, octave_value* ret[])
{
  if (n > 0)
    {
      ret[0] = new octave_value(1.0);
      return 1;
    }
  else
    return 0;
}

DEFUN_DLD(noovchurn,args,,"test array of pointers to ovs")
{
  for (int i=1; i < 10000; i++)
    {
      octave_value* ret[1];
      int rets = noovchurn(1,ret);
      if (rets) delete ret[0];
    }
  return octave_value_list();
}