[Octave-bug-tracker] [bug #62968] idx_vector constructor m_len calculation is incorret

[Maged Rifaat]

URL:
  <https://savannah.gnu.org/bugs/?62968>

                 Summary: idx_vector constructor m_len calculation is incorret
                 Project: GNU Octave
               Submitter: magedrifaat
               Submitted: Sat 27 Aug 2022 11:37:34 AM UTC
                Category: Interpreter
                Severity: 3 - Normal
                Priority: 5 - Normal
              Item Group: Incorrect Result
                  Status: None
             Assigned to: None
         Originator Name: 
        Originator Email: 
             Open/Closed: Open
                 Release: 7.2.0
         Discussion Lock: Any
        Operating System: GNU/Linux


    _______________________________________________________

Follow-up Comments:


-------------------------------------------------------
Date: Sat 27 Aug 2022 11:37:34 AM UTC By: Maged Rifaat <magedrifaat>
Among the many constructors of idx_vector, there is a constructor that accepts
start, limit and step:

idx_vector (octave_idx_type start, octave_idx_type limit,
            octave_idx_type step = 1)
  : m_rep (new idx_range_rep (start, limit, step)) { }


However using this constructor with a step of 2 or higher can result in wrong
number of elements, for example:

idx_vector i = idx_vector (1, 10, 2);

Will create a vector of 4 elements only (1, 3, 5, 7) instead of the expected 5
elements (1, 3, 5, 7, 9).

Tracing this constructor I found a calculation error in the underlying
idx_range_rep constructor:

idx_vector::idx_range_rep::idx_range_rep (octave_idx_type start,
                                          octave_idx_type limit,
                                          octave_idx_type step)
  : idx_base_rep (), m_start(start),
    m_len (step ? std::max ((limit - start) / step,
                            static_cast<octave_idx_type> (0))
           : -1),
    m_step (step)

Essentially the calculation for m_len is (limit - start) / step. I think this
is incorrect as it doesn't account for cases where the limit is not aligned
with the step.

If i got this right, the correct calculation would be a rounding up, and since
all operands are integers it can be done like this:

m_len = (limit - start + step - 1) / step;


My reasoning here is that the number of _step_ intervals existing between
start and limit needn't be a whole number because we only need the first
number in this interval so any fraction means we at least have this first
number so we can round up and count it as a whole interval.

But this doesn't account for negative _step_, so instead of rounding up we
need to round away from zero:

if (step > 0)
  m_len = (limit - start + step - 1) / step;
else
  m_len = (limit - start + step + 1) / step;


This constructor doesn't seem to be ever used with a step >= 2 anywhere in
Octave's core that I can find so I don't think any of the existing code is
affected by this bug.







    _______________________________________________________

Reply to this item at:

  <https://savannah.gnu.org/bugs/?62968>

_______________________________________________
Message sent via Savannah
https://savannah.gnu.org/