Issue 4212: fix out-of-bounds index in division_maior()

Issue 4212: fix out-of-bounds index in division_maior()

[benko.pal, 2015-01-01 22:57:31]

https://codereview.appspot.com/189420043/diff/1/lily/breathing-sign.cc
File lily/breathing-sign.cc (right):

https://codereview.appspot.com/189420043/diff/1/lily/breathing-sign.cc#newcod...
lily/breathing-sign.cc:122: if (ydim[DOWN] < val && line_pos.begin () < it - 1)
I'd rather write
line_pos.begin () + 1 < it
but good catch anyway, thanks!

[Dan Eble, 2015-01-01 23:08:56]

On 2015/01/01 22:57:31, benko.pal wrote:
> https://codereview.appspot.com/189420043/diff/1/lily/breathing-sign.cc
> File lily/breathing-sign.cc (right):
> 
>
https://codereview.appspot.com/189420043/diff/1/lily/breathing-sign.cc#newcod...
> lily/breathing-sign.cc:122: if (ydim[DOWN] < val && line_pos.begin () < it -
1)
> I'd rather write
> line_pos.begin () + 1 < it
> but good catch anyway, thanks!

I also find the whole surrounding code difficult to read, but I don't want to
spend any more time on this than I have to.  Thanks for the feedback.

[benko.pal, 2015-01-02 07:21:19]

On 2015/01/01 23:08:56, Dan Eble wrote:
> On 2015/01/01 22:57:31, benko.pal wrote:
> > https://codereview.appspot.com/189420043/diff/1/lily/breathing-sign.cc
> > File lily/breathing-sign.cc (right):
> > 
> >
>
https://codereview.appspot.com/189420043/diff/1/lily/breathing-sign.cc#newcod...
> > lily/breathing-sign.cc:122: if (ydim[DOWN] < val && line_pos.begin () < it -
> 1)
> > I'd rather write
> > line_pos.begin () + 1 < it
> > but good catch anyway, thanks!
> 
> I also find the whole surrounding code difficult to read, but I don't want to
> spend any more time on this than I have to.  Thanks for the feedback.

your code is fine, I withdraw my suggestion (I should never hurry a review).
thanks again!

[dak, 2015-01-02 08:56:03]

On 2015/01/02 07:21:19, benko.pal wrote:
> On 2015/01/01 23:08:56, Dan Eble wrote:
> > On 2015/01/01 22:57:31, benko.pal wrote:
> > > https://codereview.appspot.com/189420043/diff/1/lily/breathing-sign.cc
> > > File lily/breathing-sign.cc (right):
> > > 
> > >
> >
>
https://codereview.appspot.com/189420043/diff/1/lily/breathing-sign.cc#newcod...
> > > lily/breathing-sign.cc:122: if (ydim[DOWN] < val && line_pos.begin () < it
-
> > 1)
> > > I'd rather write
> > > line_pos.begin () + 1 < it
> > > but good catch anyway, thanks!
> > 
> > I also find the whole surrounding code difficult to read, but I don't want
to
> > spend any more time on this than I have to.  Thanks for the feedback.
> 
> your code is fine, I withdraw my suggestion (I should never hurry a review).
> thanks again!

Actually, all of those checks (existing and proposed) look like undefined
behavior since they calculate a possibly non-existing iterator and compare with
it.  With most compilers and implementations things will probably work, but one
can't really rely on it.  In particular, the compiler is allowed to make
line_pos.begin () < whatever the same as line_pos.begin () != whatever.

[benko.pal, 2015-01-02 11:53:29]

On 2015/01/02 08:56:03, dak wrote:
> On 2015/01/02 07:21:19, benko.pal wrote:
> > On 2015/01/01 23:08:56, Dan Eble wrote:
> > > On 2015/01/01 22:57:31, benko.pal wrote:
> > > > https://codereview.appspot.com/189420043/diff/1/lily/breathing-sign.cc
> > > > File lily/breathing-sign.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.appspot.com/189420043/diff/1/lily/breathing-sign.cc#newcod...
> > lily/breathing-sign.cc:122: if (ydim[DOWN] < val && line_pos.begin () < it -
1)
> > I'd rather write
> > line_pos.begin () + 1 < it
> > but good catch anyway, thanks!

> Actually, all of those checks (existing and proposed) look like undefined
> behavior since they calculate a possibly non-existing iterator and compare
with
> it.  With most compilers and implementations things will probably work, but
one
> can't really rely on it.  In particular, the compiler is allowed to make
> line_pos.begin () < whatever the same as line_pos.begin () != whatever.

it - 1 was used above as it[-1]; it exists, because ydim is a positive range
(last if) strictly smaller than the whole staff (widening by negative amount),
so it must be larger than begin, as asserted.

[Dan Eble, 2015-01-02 14:16:54]

On 2015/01/02 08:56:03, dak wrote:
> In particular, the compiler is allowed to make
> line_pos.begin () < whatever the same as line_pos.begin () != whatever.

* std::vector elements must be contiguous after C++03
(http://en.cppreference.com/w/cpp/container/vector)
* it[n] is convertible to a reference and is equivalent to *(i + n)
(http://en.cppreference.com/w/cpp/concept/RandomAccessIterator)

Therefore there should be no problem with "x.begin() < it" as long as the vector
is not empty and "it != x.end()".  I'm not sure if anything constrains the
implementation of x.end() to refer to the place this code assumes it does, so
that might still be a problem.

Still, I'm going to resist doing anything other than fixing the immediate
problem.  I do not want to risk spending a bunch of time debugging if I rewrite
it wrong.

[dak, 2015-01-02 14:36:34]

On 2015/01/02 14:16:54, Dan Eble wrote:
> On 2015/01/02 08:56:03, dak wrote:
> > In particular, the compiler is allowed to make
> > line_pos.begin () < whatever the same as line_pos.begin () != whatever.
> 
> * std::vector elements must be contiguous after C++03
> (http://en.cppreference.com/w/cpp/container/vector)
> * it[n] is convertible to a reference and is equivalent to *(i + n)
> (http://en.cppreference.com/w/cpp/concept/RandomAccessIterator)
> 
> Therefore there should be no problem with "x.begin() < it" as long as the
vector
> is not empty and "it != x.end()".  I'm not sure if anything constrains the
> implementation of x.end() to refer to the place this code assumes it does, so
> that might still be a problem.
> 
> Still, I'm going to resist doing anything other than fixing the immediate
> problem.  I do not want to risk spending a bunch of time debugging if I
rewrite
> it wrong.

The case I was worried about was (line_pos.begin () < it - 1)
This expression is undefined if it == line_pos.begin () since line_pos.begin ()
- 1 is undefined, being before the start of the array.

[Dan Eble, 2015-01-02 15:44:30]

On 2015/01/02 14:36:34, dak wrote:
> The case I was worried about was (line_pos.begin () < it - 1)
> This expression is undefined if it == line_pos.begin () since line_pos.begin
()
> - 1 is undefined, being before the start of the array.

Two lines up there is assert (line_pos.begin () < it), so it looks OK.