Re: purpose of 10486:4e64fbbd5c58

[Jaroslav Hajek]

On Thu, Apr 29, 2010 at 11:57 PM, John W. Eaton <address@hidden> wrote:
> On  6-Apr-2010, Jaroslav Hajek wrote:
>
> | I guess it would take a not-quite-trivial effort to make it exactly
> | Matlab compatible (cf. the above example). If you don't make it Matlab
> | compatible, you sort of lose the primary motivation for this whole
> | thing.
>
> I think for the purposes of the person I am working with, it would be
> OK if Octave allowed both
>
>  x(1:1.7:9)
>
> and
>
>  i = 1:1.7:9;
>  x(i)
>
> and it would also be OK if this is only enabled with --traditional.
>
> | Do as you wish if this didn't convince you, I think I've exhausted all
> | my arguments. I know you have a negative attitude towards more global
> | variables, but if it's possible, I'd like to have a way to turn this
> | stupidity off.
>
> If we use a warning with an ID, then couldn't you just turn the
> specific warning into an error?
>
> Is the following patch OK with you?

I never use --traditional, so this only means an inconsistency in
error messages for me.
The only problem is that previously, octave_value::index_vector used
to throw at error, so one could use it without checking error_state
afterwards, and I think I used that several times when coding for
Octave, so there may be some hidden errors induced by this change.

> It's based on the changeset you
> sent earlier.  With it, I see
>
>  octave:1> x = 1:10
>  x =
>
>      1    2    3    4    5    6    7    8    9   10
>
>  octave:2> x(1:1.7:9)
>  error: rounding non-integer range used as index to nearest integer
>  octave:2> isindex (1:1.7:9)
>  ans = 0
>
> in normal Octave mode, and
>
>  >> x = 1:10
>  x =
>
>      1    2    3    4    5    6    7    8    9   10
>
>  >> x(1:1.7:9)
>  warning: rounding non-integer range used as index to nearest integer
>  ans =
>
>          1        3        4        6        8
>
>  >> isindex (1:1.7:9)
>  ans = 0
>
> with --traditional.  The only thing I'm confused about is why isindex
> should be returning false for this case when using the value as an
> index works.  But isn't that what your patch did as well?
>

The reason is that if isindex (I) is true, I expect isindex ([1; I])
to be also true, expect it to be usable in sub2ind etc. If there *has*
to be an iconsistency like this, I'd prefer it to be as non-intrusive
as possible.

I understand your motivation for this, though I still consider this to
be the same kind of misfeature like the short-circuiting & and |
operators. As I said, I've exhausted my arguments. The decision is up
to you.

> jwe
>
>
>
> # HG changeset patch
> # User Jaroslav Hajek <address@hidden>
> # Date 1272577979 14400
> # Node ID c919d07ac8236b390e49b2ece224a96cd42312dd
> # Parent  eca2af8a92bcc5dc7a02b42455766b514b38556a
> allow non-integer ranges as indices conditionally
>
> diff --git a/src/ChangeLog b/src/ChangeLog
> --- a/src/ChangeLog
> +++ b/src/ChangeLog
> @@ -1,3 +1,18 @@
> +2010-04-29  Jaroslav Hajek  <address@hidden>
> +           John W. Eaton  <address@hidden>
> +
> +       * utils.cc (reset_warning_state): New function.
> +       (Fisindex): Temporarily set warning state for
> +       Octave:allow-noninteger-ranges-as-indices to "error".
> +       * error.cc (set_warning_state): New function.
> +       (initialize_default_warning_state): Set default warning state
> +       for Octave:allow-noninteger-ranges-as-indices to "error".
> +       * error.h (set_warning_state): Provide decl.
> +       * octave.cc (maximum_braindamage): Set warning state for
> +       Octave:allow-noninteger-ranges-as-indices to "on".
> +       * ov-range.cc (octave_range::index_vector): Warn if range
> +       contains non-integer values.
> +
>  2010-04-27  John W. Eaton  <address@hidden>
>
>        * graphics.h.in (string_array_property::string_array_property):
> diff --git a/src/error.cc b/src/error.cc
> --- a/src/error.cc
> +++ b/src/error.cc
> @@ -1441,17 +1441,23 @@
>  }
>
>  void
> -disable_warning (const std::string& id)
> +set_warning_state (const std::string& id, const std::string& state)
>  {
>   octave_value_list args;
>
>   args(1) = id;
> -  args(0) = "off";
> +  args(0) = state;
>
>   Fwarning (args, 0);
>  }
>
>  void
> +disable_warning (const std::string& id)
> +{
> +  set_warning_state (id, "off");
> +}
> +
> +void
>  initialize_default_warning_state (void)
>  {
>   initialize_warning_options ("on");
> @@ -1473,6 +1479,10 @@
>   disable_warning ("Octave:string-concat");
>   disable_warning ("Octave:variable-switch-label");
>   disable_warning ("Octave:complex-cmp-ops");
> +
> +  // This should be an error unless we are in maximum braindamage mode.
> +
> +  set_warning_state ("Octave:allow-noninteger-ranges-as-indices", "error");
>  }
>
>  DEFUN (lasterror, args, ,
> diff --git a/src/error.h b/src/error.h
> --- a/src/error.h
> +++ b/src/error.h
> @@ -94,6 +94,9 @@
>  // Helper function for print_usage defined in defun.cc.
>  extern OCTINTERP_API void defun_usage_message (const std::string& msg);
>
> +extern OCTINTERP_API void
> +set_warning_state (const std::string& id, const std::string& state);
> +
>  extern OCTINTERP_API void disable_warning (const std::string& id);
>  extern OCTINTERP_API void initialize_default_warning_state (void);
>
> diff --git a/src/octave.cc b/src/octave.cc
> --- a/src/octave.cc
> +++ b/src/octave.cc
> @@ -573,6 +573,8 @@
>   bind_internal_variable ("page_screen_output", false);
>   bind_internal_variable ("print_empty_dimensions", false);
>
> +  set_warning_state ("Octave:allow-noninteger-ranges-as-indices", "on");
> +
>   disable_warning ("Octave:abbreviated-property-match");
>   disable_warning ("Octave:fopen-file-in-path");
>   disable_warning ("Octave:function-name-clash");
> diff --git a/src/ov-range.cc b/src/ov-range.cc
> --- a/src/ov-range.cc
> +++ b/src/ov-range.cc
> @@ -30,6 +30,8 @@
>  #include "lo-ieee.h"
>  #include "lo-utils.h"
>
> +#include "defun.h"
> +#include "variables.h"
>  #include "gripes.h"
>  #include "ops.h"
>  #include "oct-obj.h"
> @@ -142,6 +144,25 @@
>     }
>  }
>
> +idx_vector
> +octave_range::index_vector (void) const
> +{
> +  if (idx_cache)
> +    return *idx_cache;
> +  else
> +    {
> +      if (range.all_elements_are_ints ())
> +        return set_idx_cache (idx_vector (range));
> +      else
> +        {
> +          warning_with_id ("Octave:allow-noninteger-ranges-as-indices",
> +                           "rounding non-integer range used as index to 
> nearest integer");
> +
> +          return octave_value (matrix_value ()).round ().index_vector ();
> +        }
> +    }
> +}
> +
>  double
>  octave_range::double_value (bool) const
>  {
> diff --git a/src/ov-range.h b/src/ov-range.h
> --- a/src/ov-range.h
> +++ b/src/ov-range.h
> @@ -106,8 +106,7 @@
>   octave_value do_index_op (const octave_value_list& idx,
>                             bool resize_ok = false);
>
> -  idx_vector index_vector (void) const
> -    { return idx_cache ? *idx_cache : set_idx_cache (idx_vector (range)); }
> +  idx_vector index_vector (void) const;
>
>   dim_vector dims (void) const
>     {
> diff --git a/src/utils.cc b/src/utils.cc
> --- a/src/utils.cc
> +++ b/src/utils.cc
> @@ -60,6 +60,7 @@
>  #include "oct-hist.h"
>  #include "oct-obj.h"
>  #include "pager.h"
> +#include "parse.h"
>  #include "sysdep.h"
>  #include "toplev.h"
>  #include "unwind-prot.h"
> @@ -1303,6 +1304,17 @@
>     }
>  }
>
> +// FIXME -- is there some way to fix the declarations in unwind-prot.h
> +// so that this function's argument can be declared as
> +// "const octave_value_list&"?
> +
> +static void
> +reset_warning_state (octave_value_list args)
> +{
> +  if (! args.empty ())
> +    feval ("warning", args, 0);
> +}
> +
>  DEFUN (isindex, args, ,
>   "-*- texinfo -*-\n\
> address@hidden {Built-in Function} {} isindex (@var{ind}, @var{n})\n\
> @@ -1325,7 +1337,18 @@
>   if (! error_state)
>     {
>       unwind_protect frame;
> +
> +      octave_value_list xargs;
> +
> +      xargs(1) = "Octave:allow-noninteger-ranges-as-indices";
> +      xargs(0) = "error";
> +
> +      octave_value_list current_warning_state = feval ("warning", xargs, 1);
> +
> +      frame.add_fcn (reset_warning_state, current_warning_state);
> +
>       frame.protect_var (error_state);
> +
>       frame.protect_var (discard_error_messages);
>       discard_error_messages = true;
>
>
>



-- 
RNDr. Jaroslav Hajek, PhD
computing expert & GNU Octave developer
Aeronautical Research and Test Institute (VZLU)
Prague, Czech Republic
url: www.highegg.matfyz.cz