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

[David Spies]

On Tue, Jun 24, 2014 at 2:00 PM, John W. Eaton <address@hidden> wrote:

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_cast is for going the other way from const to unconst.  It's inherently unsafe and should be avoided when possible.  This cast is only to specify we want to call the const version of xelem (which doesn't require the the reference be unique like the unconst version does).

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

work here anyway, no cast required?

Yes, if you think that it's cleaner to do this as an implicit cast I'll change it.
 

+#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.

Probably, but all the other debugging options are checked with #if(defined(OPT)) etc
If bounds-checking behaved differently, someone might accidentally use #if(defined(BOUNDS_CHECKING)) and then end up with code that's slow because bounds-checking is always defined.
Additionally, adapting to this change would be more work because I'd have to change everywhere where BOUNDS_CHECKING is used.  This seemed simpler and safer.

 

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

Sure

 

+  // 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.

I didn't know that.  How do I add a new configure option?  Also, why are asserts always enabled?
 

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

These were all cases where a call to unconst xelem resulted in the is_unique assert failing because there was a second assignment to the same array data.  Rather than casting to const to call the right xelem (which looks a little bulky), I thought I could silently just move the second assignment to after the call to xelem.  It works, but I suppose it's not really the proper thing to do.  I should just cast to const instead.

jwe