Re: Should xelem bounds-check (and reference-check) when debugging options are on?

[John W. Eaton]

On 06/23/2014 02:06 PM, David Spies wrote:
> Here's my proposed changeset for this (labeled "Added safety checks to
> Array::xelem"):
>
> http://hg.octave.org/octave-dspies/graph/
>
> Thanks,
> David
>
>
> On Fri, Jun 20, 2014 at 11:42 AM, David Spies <address@hidden
> <mailto:address@hidden>> wrote:
>
>     Hello,
>
>     When constructing (or accessing) matrix elements, I'd like to be
>     able to call a function which does bounds-checking (and in the case
>     of assignment, reference-count-checking with an exception if there's
>     more than one reference) only when debugging options are turned on.
>
>     I'm proposing adding these checks to the xelem method instead of
>     creating a new method for it.  I can't imagine why it's necessary to
>     have a method that does "no checks of any kind ever! Not even when
>     debugging!"
>
>     What do people think of the idea?
>
>     Thanks,
>     David

I assume you mean changeset 8bcfea54ce19, correct?

I think it is OK to allow bounds and reference checking to be
optionally enabled for the xelem functions, but they should remain
disabled by default.

+++ b/libinterp/corefcn/jit-typeinfo.cc
@@ -287,7 +287,7 @@
       make_indices (indicies, idx_count, idx);

       Array<double> ret = mat->array->index (idx);
-      return ret.xelem (0);
+      return (static_cast<const Array<double>&> (ret)).xelem (0);

If you need to change the const-ness of something, shouldn't you use
const_cast?  Or wouldn't

  const Array<double>& ret = mat->array->index (idx);

work here anyway, no cast required?

+#if defined(BOUNDS_CHECKING)
+#define BOUNDS_CHECKING_DEFINED true
+#else
+#define BOUNDS_CHECKING_DEFINED false
+#endif

It might be better to arrange to always define BOUNDS_CHECKING to 1 or
0 (or true or false) to avoid the need for this extra definition.

Instead of "check_out_of_range", perhaps use "check_index_bounds"?

+  // Check for multiple references only if asserts are enabled
+  T&
+  xelem (octave_idx_type n)
+  {
+    assert(is_unique ());

I think asserts are always enabled, aren't they?  We should probably
have a configure option to enable this check.  I'm not sure what name
to use.

The changes to dbleQR.cc, floatQR.cc, CmplxQR.cc, and fCmplx.cc seem
unrelated, so should be deleted from the changeset.

jwe