changeset 10366:e5ae13b8b2c2

improve Array indexing error messages
author Jaroslav Hajek <highegg@gmail.com>
date Sat, 27 Feb 2010 08:37:34 +0100
parents 532802559f39
children 173e10268080
files liboctave/Array-util.cc liboctave/Array-util.h liboctave/Array.cc liboctave/Array.h liboctave/ChangeLog liboctave/DiagArray2.cc liboctave/Range.cc liboctave/dim-vector.h src/ChangeLog src/ov-base-mat.cc test/test_struct.m
diffstat 11 files changed, 198 insertions(+), 188 deletions(-) [+]
line wrap: on
line diff
--- a/liboctave/Array-util.cc
+++ b/liboctave/Array-util.cc
@@ -641,8 +641,8 @@
 }
 
 void
-gripe_nonconformant (const char *op, dim_vector& op1_dims,
-                     dim_vector& op2_dims)
+gripe_nonconformant (const char *op, const dim_vector& op1_dims,
+                     const dim_vector& op2_dims)
 {
   std::string op1_dims_str = op1_dims.str ();
   std::string op2_dims_str = op2_dims.str ();
@@ -651,3 +651,34 @@
     ("%s: nonconformant arguments (op1 is %s, op2 is %s)",
      op, op1_dims_str.c_str (), op2_dims_str.c_str ());
 }
+
+void gripe_index_out_of_range (int nd, int dim, 
+                               octave_idx_type idx, octave_idx_type ext)
+{
+  switch (nd)
+    {
+    case 1:
+      (*current_liboctave_error_handler)
+        ("A(I): index out of bounds; value %d out of bound %d",
+         idx, ext);
+      break;
+    case 2:
+      (*current_liboctave_error_handler)
+        ("A(I,J): %s index out of bounds; value %d out of bound %d",
+         (dim == 1) ? "row" : "column", idx, ext);
+      break;
+    default:
+      (*current_liboctave_error_handler)
+        ("A(I,J,...): index to dimension %d out of bounds; value %d out of bound %d",
+         dim, idx, ext);
+      break;
+    }
+}
+
+void gripe_del_index_out_of_range (bool is1d, octave_idx_type idx, 
+                                   octave_idx_type ext)
+{
+  (*current_liboctave_error_handler)
+    ("A(%s) = []: index out of bounds; value %d out of bound %d",
+     is1d ? "I" : "..,I,..", idx, ext);
+}
--- a/liboctave/Array-util.h
+++ b/liboctave/Array-util.h
@@ -99,7 +99,13 @@
                                  int op2_nr, int op2_nc);
 
 
-extern void OCTAVE_API gripe_nonconformant (const char *op, dim_vector& op1_dims,
-                                 dim_vector& op2_dims);
+extern void OCTAVE_API gripe_nonconformant (const char *op, const dim_vector& op1_dims,
+                                            const dim_vector& op2_dims);
+
+extern void OCTAVE_API gripe_index_out_of_range (int nd, int dim, 
+                                                 octave_idx_type iext, octave_idx_type ext);
+
+extern void OCTAVE_API gripe_del_index_out_of_range (bool is1d, octave_idx_type iext, 
+                                                     octave_idx_type ext);
 
 #endif
--- a/liboctave/Array.cc
+++ b/liboctave/Array.cc
@@ -206,60 +206,102 @@
 }
 
 template <class T>
-T&
-Array<T>::range_error (const char *fcn, octave_idx_type n) const
+T& 
+Array<T>::checkelem (octave_idx_type n)
 {
-  (*current_liboctave_error_handler) ("%s (%d): range error", fcn, n);
-  static T foo;
-  return foo;
+  if (n < 0 || n >= slice_len)
+    gripe_index_out_of_range (1, 1, n+1, slice_len);
+
+  return elem (n);
 }
 
 template <class T>
-T&
-Array<T>::range_error (const char *fcn, octave_idx_type i, octave_idx_type j) const
+T& 
+Array<T>::checkelem (octave_idx_type i, octave_idx_type j)
 {
-  (*current_liboctave_error_handler)
-    ("%s (%d, %d): range error", fcn, i, j);
-  static T foo;
-  return foo;
+  if (i < 0 || i >= dim1 ())
+    gripe_index_out_of_range (2, 1, i+1, dim1 ());
+  if (j < 0 || j >= dimensions.numel (1))
+    gripe_index_out_of_range (2, 2, j+1, dimensions.numel (1));
+
+  return elem (i, j);
+}
+
+template <class T>
+T& 
+Array<T>::checkelem (octave_idx_type i, octave_idx_type j, octave_idx_type k)
+{
+  if (i < 0 || i >= dim1 ())
+    gripe_index_out_of_range (3, 1, i+1, dim1 ());
+  if (j < 0 || j >= dim2 ())
+    gripe_index_out_of_range (3, 2, j+1, dim2 ());
+  if (k < 0 || k >= dimensions.numel (2))
+    gripe_index_out_of_range (3, 3, k+1, dimensions.numel (2));
+
+  return elem (i, j, k);
 }
 
 template <class T>
-T&
-Array<T>::range_error (const char *fcn, octave_idx_type i, octave_idx_type j, octave_idx_type k) const
+T& 
+Array<T>::checkelem (const Array<octave_idx_type>& ra_idx)
 {
-  (*current_liboctave_error_handler)
-    ("%s (%d, %d, %d): range error", fcn, i, j, k);
-  static T foo;
-  return foo;
+  int nd = ra_idx.length ();
+  const dim_vector dv = dimensions.redim (nd);
+  for (int d = 0; d < nd; d++)
+    if (ra_idx(d) < 0 || ra_idx(d) >= dv(d))
+      gripe_index_out_of_range (nd, d+1, ra_idx(d)+1, dv(d));
+
+  return elem (ra_idx);
+}
+
+template <class T>
+typename Array<T>::crefT
+Array<T>::checkelem (octave_idx_type n) const
+{
+  if (n < 0 || n >= slice_len)
+    gripe_index_out_of_range (1, 1, n+1, slice_len);
+
+  return elem (n);
 }
 
 template <class T>
-T&
-Array<T>::range_error (const char *fcn, const Array<octave_idx_type>& ra_idx) const
+typename Array<T>::crefT
+Array<T>::checkelem (octave_idx_type i, octave_idx_type j) const
+{
+  if (i < 0 || i >= dim1 ())
+    gripe_index_out_of_range (2, 1, i+1, dim1 ());
+  if (j < 0 || j >= dimensions.numel (1))
+    gripe_index_out_of_range (2, 2, j+1, dimensions.numel (1));
+
+  return elem (i, j);
+}
+
+template <class T>
+typename Array<T>::crefT
+Array<T>::checkelem (octave_idx_type i, octave_idx_type j, octave_idx_type k) const
 {
-  std::ostringstream buf;
-
-  buf << fcn << " (";
-
-  octave_idx_type n = ra_idx.length ();
-
-  if (n > 0)
-    buf << ra_idx(0);
-
-  for (octave_idx_type i = 1; i < n; i++)
-    buf << ", " << ra_idx(i);
-
-  buf << "): range error";
-
-  std::string buf_str = buf.str ();
-
-  (*current_liboctave_error_handler) (buf_str.c_str ());
-
-  static T foo;
-  return foo;
+  if (i < 0 || i >= dim1 ())
+    gripe_index_out_of_range (3, 1, i+1, dim1 ());
+  if (j < 0 || j >= dim2 ())
+    gripe_index_out_of_range (3, 2, j+1, dim2 ());
+  if (k < 0 || k >= dimensions.numel (2))
+    gripe_index_out_of_range (3, 3, k+1, dimensions.numel (2));
+
+  return elem (i, j, k);
 }
 
+template <class T>
+typename Array<T>::crefT
+Array<T>::checkelem (const Array<octave_idx_type>& ra_idx) const
+{
+  int nd = ra_idx.length ();
+  const dim_vector dv = dimensions.redim (nd);
+  for (int d = 0; d < nd; d++)
+    if (ra_idx(d) < 0 || ra_idx(d) >= dv(d))
+      gripe_index_out_of_range (nd, d+1, ra_idx(d)+1, dv(d));
+
+  return elem (ra_idx);
+}
 
 template <class T>
 Array<T>
@@ -267,8 +309,8 @@
 {
   octave_idx_type r = dimensions(0);
 #ifdef BOUNDS_CHECKING
-  if (k < 0 || k * r >= numel ())
-    range_error ("column", k);
+  if (k < 0 || k > dimensions.numel (1))
+    gripe_index_out_of_range (2, 2, k+1, dimensions.numel (1));
 #endif
 
   return Array<T> (*this, dim_vector (r, 1), k*r, k*r + r);
@@ -280,8 +322,8 @@
 {
   octave_idx_type r = dimensions(0), c = dimensions (1), p = r*c;
 #ifdef BOUNDS_CHECKING
-  if (k < 0 || k * p >= numel ())
-    range_error ("page", k);
+  if (k < 0 || k > dimensions.numel (2))
+    gripe_index_out_of_range (3, 3, k+1, dimensions.numel (2));
 #endif
 
   return Array<T> (*this, dim_vector (r, c), k*p, k*p + p);
@@ -300,8 +342,10 @@
 Array<T>::linear_slice (octave_idx_type lo, octave_idx_type up) const
 {
 #ifdef BOUNDS_CHECKING
-  if (lo < 0 || up > numel ())
-    range_error ("linear_slice", lo, up);
+  if (lo < 0)
+    gripe_index_out_of_range (1, 1, lo+1, numel ());
+  if (up > numel ())
+    gripe_index_out_of_range (1, 1, up, numel ());
 #endif
   if (up < lo) up = lo;
   return Array<T> (*this, dim_vector (up - lo, 1), lo, up);
@@ -688,12 +732,6 @@
 
 };
 
-static void gripe_index_out_of_range (void)
-{
-  (*current_liboctave_error_handler)
-    ("A(I): Index exceeds matrix dimension.");
-}
-
 template <class T>
 Array<T>
 Array<T>::index (const idx_vector& i) const
@@ -706,12 +744,11 @@
       // A(:) produces a shallow copy as a column vector.
       retval = Array<T> (*this, dim_vector (n, 1));
     }
-  else if (i.extent (n) != n)
-    {
-      gripe_index_out_of_range ();
-    }
   else
     {
+      if (i.extent (n) != n)
+        gripe_index_out_of_range (1, 1, i.extent (n), n); // throws
+
       // FIXME -- this is the only place where orig_dimensions are used.
       dim_vector rd = i.orig_dimensions ();
       octave_idx_type il = i.length (n);
@@ -773,12 +810,13 @@
       // A(:,:) produces a shallow copy.
       retval = Array<T> (*this, dv);
     }
-  else if (i.extent (r) != r || j.extent (c) != c)
-    {
-      gripe_index_out_of_range ();
-    }
   else
     {
+      if (i.extent (r) != r)
+        gripe_index_out_of_range (2, 1, i.extent (r), r); // throws
+      if (j.extent (c) != c)
+        gripe_index_out_of_range (2, 2, i.extent (c), c); // throws
+
       octave_idx_type n = numel (), il = i.length (r), jl = j.length (c);
 
       idx_vector ii (i);
@@ -831,24 +869,17 @@
       dim_vector dv = dimensions.redim (ial);
 
       // Check for out of bounds conditions.
-      bool all_colons = true, mismatch = false;
+      bool all_colons = true;
       for (int i = 0; i < ial; i++)
         {
           if (ia(i).extent (dv(i)) != dv(i))
-            {
-              mismatch = true;
-              break;
-            }
-          else
-            all_colons = all_colons && ia(i).is_colon ();
+            gripe_index_out_of_range (ial, i+1, ia(i).extent (dv(i)), dv(i)); // throws
+
+          all_colons = all_colons && ia(i).is_colon ();
         }
 
 
-      if (mismatch)
-        {
-          gripe_index_out_of_range ();
-        }
-      else if (all_colons)
+      if (all_colons)
         {
           // A(:,:,...,:) produces a shallow copy.
           dv.chop_trailing_singletons ();
@@ -1387,12 +1418,11 @@
     { 
       *this = Array<T> ();
     }
-  else if (i.extent (n) != n)
-    {
-      gripe_index_out_of_range ();
-    }
   else if (i.length (n) != 0)
     {
+      if (i.extent (n) != n)
+        gripe_del_index_out_of_range (true, i.extent (n), n);
+
       octave_idx_type l, u;
       bool col_vec = ndims () == 2 && columns () == 1 && rows () != 1;
       if (i.is_scalar () && i(0) == n-1)
@@ -1435,12 +1465,11 @@
     { 
       *this = Array<T> ();
     }
-  else if (i.extent (n) != n)
-    {
-      gripe_index_out_of_range ();
-    }
   else if (i.length (n) != 0)
     {
+      if (i.extent (n) != n)
+        gripe_del_index_out_of_range (false, i.extent (n), n);
+
       octave_idx_type l, u;
 
       if (i.is_cont_range (n, l, u))
--- a/liboctave/Array.h
+++ b/liboctave/Array.h
@@ -331,13 +331,10 @@
   void chop_trailing_singletons (void) GCC_ATTR_DEPRECATED
   { dimensions.chop_trailing_singletons (); }
   
+  octave_idx_type compute_index (octave_idx_type i, octave_idx_type j) const;
+  octave_idx_type compute_index (octave_idx_type i, octave_idx_type j, octave_idx_type k) const;
   octave_idx_type compute_index (const Array<octave_idx_type>& ra_idx) const;
 
-  T& range_error (const char *fcn, octave_idx_type n) const;
-  T& range_error (const char *fcn, octave_idx_type i, octave_idx_type j) const;
-  T& range_error (const char *fcn, octave_idx_type i, octave_idx_type j, octave_idx_type k) const;
-  T& range_error (const char *fcn, const Array<octave_idx_type>& ra_idx) const;
-
   // No checking, even for multiple references, ever.
 
   T& xelem (octave_idx_type n) { return slice_data [n]; }
@@ -361,42 +358,10 @@
   // unnecessarily force a copy, but that is not so easy, and I see no
   // clean way to do it.
 
-  T& checkelem (octave_idx_type n)
-    {
-      if (n < 0 || n >= slice_len)
-        return range_error ("T& Array<T>::checkelem", n);
-      else
-        {
-          make_unique ();
-          return xelem (n);
-        }
-    }
-
-  T& checkelem (octave_idx_type i, octave_idx_type j)
-    {
-      if (i < 0 || j < 0 || i >= dim1 () || j >= dim2 ())
-        return range_error ("T& Array<T>::checkelem", i, j);
-      else
-        return elem (dim1()*j+i);
-    }
-
-  T& checkelem (octave_idx_type i, octave_idx_type j, octave_idx_type k)
-    {
-      if (i < 0 || j < 0 || k < 0 || i >= dim1 () || j >= dim2 () || k >= dim3 ())
-        return range_error ("T& Array<T>::checkelem", i, j, k);
-      else
-        return elem (i, dim2()*k+j);
-    }
-
-  T& checkelem (const Array<octave_idx_type>& ra_idx)
-    {
-      octave_idx_type i = compute_index (ra_idx);
-
-      if (i < 0)
-        return range_error ("T& Array<T>::checkelem", ra_idx);
-      else
-        return elem (i);
-    }
+  T& checkelem (octave_idx_type n);
+  T& checkelem (octave_idx_type i, octave_idx_type j);
+  T& checkelem (octave_idx_type i, octave_idx_type j, octave_idx_type k);
+  T& checkelem (const Array<octave_idx_type>& ra_idx);
 
   T& elem (octave_idx_type n)
     {
@@ -423,48 +388,19 @@
   T& operator () (const Array<octave_idx_type>& ra_idx) { return elem (ra_idx); }
 #endif
 
-  crefT checkelem (octave_idx_type n) const
-    {
-      if (n < 0 || n >= slice_len)
-        return range_error ("T Array<T>::checkelem", n);
-      else
-        return xelem (n);
-    }
-
-  crefT checkelem (octave_idx_type i, octave_idx_type j) const
-    {
-      if (i < 0 || j < 0 || i >= dim1 () || j >= dim2 ())
-        return range_error ("T Array<T>::checkelem", i, j);
-      else
-        return elem (dim1()*j+i);
-    }
-
-  crefT checkelem (octave_idx_type i, octave_idx_type j, octave_idx_type k) const
-    {
-      if (i < 0 || j < 0 || k < 0 || i >= dim1 () || j >= dim2 () || k >= dim3 ())
-        return range_error ("T Array<T>::checkelem", i, j, k);
-      else
-        return Array<T>::elem (i, Array<T>::dim1()*k+j);
-    }
-
-  crefT checkelem (const Array<octave_idx_type>& ra_idx) const
-    {
-      octave_idx_type i = compute_index (ra_idx);
-
-      if (i < 0)
-        return range_error ("T Array<T>::checkelem", ra_idx);
-      else
-        return Array<T>::elem (i);
-    }
+  crefT checkelem (octave_idx_type n) const;
+  crefT checkelem (octave_idx_type i, octave_idx_type j) const;
+  crefT checkelem (octave_idx_type i, octave_idx_type j, octave_idx_type k) const;
+  crefT checkelem (const Array<octave_idx_type>& ra_idx) const;
 
   crefT elem (octave_idx_type n) const { return xelem (n); }
 
-  crefT elem (octave_idx_type i, octave_idx_type j) const { return elem (dim1()*j+i); }
+  crefT elem (octave_idx_type i, octave_idx_type j) const { return xelem (i, j); }
 
-  crefT elem (octave_idx_type i, octave_idx_type j, octave_idx_type k) const { return elem (i, dim2()*k+j); }
+  crefT elem (octave_idx_type i, octave_idx_type j, octave_idx_type k) const { return xelem (i, j, k); }
 
   crefT elem (const Array<octave_idx_type>& ra_idx) const
-    { return Array<T>::elem (compute_index (ra_idx)); }
+    { return Array<T>::xelem (compute_index (ra_idx)); }
 
 #if defined (BOUNDS_CHECKING)
   crefT operator () (octave_idx_type n) const { return checkelem (n); }
--- a/liboctave/ChangeLog
+++ b/liboctave/ChangeLog
@@ -1,3 +1,12 @@
+2010-02-27  Jaroslav Hajek  <highegg@gmail.com>
+
+	* Array-util.cc (gripe_index_out_of_range): New function.
+	* Array.cc (Array<T>::range_error): Remove.
+	(Array<T>::checkelem): Rewrite.
+	(Array<T>::index, Array<T>::delete_elements): Simplify.
+	* DiagArray2.cc (DiagArray2::checkelem): Use gripe_index_out_of_range.
+	* Range.cc (Range::checkelem, Range::index): Ditto.
+
 2010-02-26  Jaroslav Hajek  <highegg@gmail.com>
 
 	* mx-inlines.cc (OP_DUP_FCN): Remove.
--- a/liboctave/DiagArray2.cc
+++ b/liboctave/DiagArray2.cc
@@ -85,8 +85,10 @@
 T
 DiagArray2<T>::checkelem (octave_idx_type r, octave_idx_type c) const
 {
-  if (r < 0 || c < 0 || r >= dim1 () || c >= dim2 ())
-    (*current_liboctave_error_handler) ("range error in DiagArray2");
+  if (r < 0 || r >= dim1 ())
+    gripe_index_out_of_range (2, 1, r+1, dim1 ());
+  if (c < 0 || c >= dim2 ())
+    gripe_index_out_of_range (2, 2, c+1, dim2 ());
 
   return elem (r, c);
 }
--- a/liboctave/Range.cc
+++ b/liboctave/Range.cc
@@ -35,6 +35,7 @@
 #include "lo-mappers.h"
 #include "lo-math.h"
 #include "lo-utils.h"
+#include "Array-util.h"
 
 Range::Range (double b, double i, octave_idx_type n)
   : rng_base (b), rng_limit (b + n * i), rng_inc (i), 
@@ -85,7 +86,7 @@
 Range::checkelem (octave_idx_type i) const
 {
   if (i < 0 || i >= rng_nelem)
-    (*current_liboctave_error_handler) ("Range::elem (%d): range error", i);
+    gripe_index_out_of_range (1, 1, i+1, rng_nelem);
 
   return rng_base + rng_inc * i;
 }
@@ -110,13 +111,11 @@
     {
       retval = matrix_value ().reshape (dim_vector (rng_nelem, 1));
     }
-  else if (i.extent (n) != n)
-    {
-      (*current_liboctave_error_handler)
-        ("A(I): Index exceeds matrix dimension.");
-    }
   else
     {
+      if (i.extent (n) != n)
+        gripe_index_out_of_range (1, 1, i.extent (n), n); // throws
+
       dim_vector rd = i.orig_dimensions ();
       octave_idx_type il = i.length (n);
 
--- a/liboctave/dim-vector.h
+++ b/liboctave/dim-vector.h
@@ -157,14 +157,18 @@
 
   octave_idx_type& elem (int i)
   {
+#ifdef BOUNDS_CHECKING
     assert (i >= 0 && i < ndims ());
+#endif
     make_unique ();
     return rep[i];
   }
 
   octave_idx_type elem (int i) const
   {
+#ifdef BOUNDS_CHECKING
     assert (i >= 0 && i < ndims ());
+#endif
     return rep[i];
   }
 
--- a/src/ChangeLog
+++ b/src/ChangeLog
@@ -1,3 +1,8 @@
+2010-02-27  Jaroslav Hajek  <highegg@gmail.com>
+
+	* ov-base-mat.cc (do_index_op): Use checkelem for scalar indices.
+	Simplify.
+
 2010-02-26  Jaroslav Hajek  <highegg@gmail.com>
 
 	* src/xpow.cc: Update.
--- a/src/ov-base-mat.cc
+++ b/src/ov-base-mat.cc
@@ -136,6 +136,7 @@
   octave_idx_type n_idx = idx.length ();
 
   int nd = matrix.ndims ();
+  const MT& cmatrix = matrix;
 
   switch (n_idx)
     {
@@ -150,8 +151,8 @@
         if (! error_state)
           {
             // optimize single scalar index.
-            if (i.is_scalar () && i(0) < matrix.numel ())
-              retval = const_cast<const MT&> (matrix)(i(0));
+            if (! resize_ok && i.is_scalar ())
+              retval = cmatrix.checkelem (i(0));
             else
               retval = MT (matrix.index (i, resize_ok));
           }
@@ -169,9 +170,8 @@
             if (! error_state)
               {
                 // optimize two scalar indices.
-                if (i.is_scalar () && j.is_scalar () && nd == 2
-                    && i(0) < matrix.rows () && j(0) < matrix.columns ())
-                  retval = const_cast<const MT&> (matrix)(i(0), j(0));
+                if (! resize_ok && i.is_scalar () && j.is_scalar ())
+                  retval = cmatrix.checkelem (i(0), j(0));
                 else
                   retval = MT (matrix.index (i, j, resize_ok));
               }
@@ -182,7 +182,7 @@
     default:
       {
         Array<idx_vector> idx_vec (n_idx, 1);
-        bool scalar_opt = n_idx == nd;
+        bool scalar_opt = n_idx == nd && ! resize_ok;
         const dim_vector dv = matrix.dims ();
 
         for (octave_idx_type i = 0; i < n_idx; i++)
@@ -192,24 +192,13 @@
             if (error_state)
               break;
 
-            scalar_opt = (scalar_opt && idx_vec(i).is_scalar ()
-                          && idx_vec(i)(0) < dv(0));
+            scalar_opt = (scalar_opt && idx_vec(i).is_scalar ());
           }
 
         if (! error_state)
           {
             if (scalar_opt)
-              {
-                // optimize all scalar indices. Don't construct an index array,
-                // but rather calc a scalar index directly.
-                octave_idx_type k = 1, j = 0;
-                for (octave_idx_type i = 0; i < n_idx; i++)
-                  {
-                    j += idx_vec(i)(0) * k;
-                    k *= dv (i);
-                  }
-                retval = const_cast<const MT&> (matrix)(j);
-              }
+              retval = cmatrix.checkelem (conv_to_int_array (idx_vec));
             else
               retval = MT (matrix.index (idx_vec, resize_ok));
           }
--- a/test/test_struct.m
+++ b/test/test_struct.m
@@ -261,4 +261,4 @@
 %!error <Index exceeds matrix dimension>
 %!  s = resize(struct(),3,2);
 %!  s(3).foo = 42;
-%!  s(7);
\ No newline at end of file
+%!  s(7);