optimal-page-breaking.cc protect unsigned subtraction; issue 1553

description

[lemzwerg, 2013-11-14 05:50:07]

LGTM.

[janek, 2013-11-17 14:31:28]

LGTM.

I think that this would warrant a comment (it's quite surprising that
min_sys_count could ever be negative), but i don't insist.

thanks,
Janek

[dak, 2013-11-20 08:53:24]

https://codereview.appspot.com/22750045/diff/1/lily/optimal-page-breaking.cc
File lily/optimal-page-breaking.cc (right):

https://codereview.appspot.com/22750045/diff/1/lily/optimal-page-breaking.cc#...
lily/optimal-page-breaking.cc:120: for (vsize sys_count = ideal_sys_count + 1;
--sys_count >= (vsize) min_sys_count;)
This was labelled as signed/unsigned bug.  If min_sys_count would happen to be
negative, this loop will never get run at all since
(vsize)-1
is a very large positive number.

That does not seem correct.

[dak, 2013-11-21 09:34:46]

https://codereview.appspot.com/22750045/diff/20001/lily/optimal-page-breaking.cc
File lily/optimal-page-breaking.cc (right):

https://codereview.appspot.com/22750045/diff/20001/lily/optimal-page-breaking...
lily/optimal-page-breaking.cc:78: if (min_sys_count > ideal_sys_count)  //
subtraction wrapped around
How about
if (min_sys_count > ideal_sys_count // subtraction wrapped around
   || min_sys_count <= 0)
  min_sys_count = 1;

Saves you the min thing.  <= instead of == just so that we don't really need to
know that vsize is unsigned.

[Keith, 2013-11-22 04:29:01]

On Thu, 21 Nov 2013 01:34:46 -0800, <dak@gnu.org> wrote:

> How about
> if (min_sys_count > ideal_sys_count // subtraction wrapped around
>     || min_sys_count <= 0)
>    min_sys_count = 1;
>
> Saves you the min thing.

That is compact, but I want to keep the min thing separate for code history. 
The two fixups of min_sys_count came from different people at different times.

This code is not run often enough that I worry about an extra test and branch.

[dak, 2013-11-22 06:08:03]

"Keith OHara" <k-ohara5a5a@oco.net> writes:

> On Thu, 21 Nov 2013 01:34:46 -0800, <dak@gnu.org> wrote:
>
>> How about
>> if (min_sys_count > ideal_sys_count // subtraction wrapped around
>>     || min_sys_count <= 0)
>>    min_sys_count = 1;
>>
>> Saves you the min thing.
>
> That is compact, but I want to keep the min thing separate for code
> history.  The two fixups of min_sys_count came from different people
> at different times.

So what?

> This code is not run often enough that I worry about an extra test and
branch.

I worry about robustness and readability.  This checks min_syscount to
be in range and corrects it if is out of range.  The current proposal
takes two different operations, bounces it back and forth, and needs a
min operation with casts.

-- 
David Kastrup