simple-spacer: inappropriate assert; issue 4448

simple-spacer: inappropriate assert; issue 4448

[lemzwerg, 2015-06-20 06:55:43]

LGTM

[dak, 2015-06-20 06:59:11]

https://codereview.appspot.com/249820043/diff/1/lily/simple-spacer.cc
File lily/simple-spacer.cc (right):

https://codereview.appspot.com/249820043/diff/1/lily/simple-spacer.cc#newcode199
lily/simple-spacer.cc:199: programming_error ("misuse of expand_line");
Any sensible fallback behavior for this "misuse of expand_line"?  If we can
reliably guess what the intent of the "misuse" is (after all, it would appear
that the situation comes about from reasonably regular input rather than
explicit user error), one could just do that and forget about the programming
error.

Is there something reasonably "continuous" one can do so that small errors lead
to small offsets?

[Keith, 2015-06-21 02:08:49]

On Fri, 19 Jun 2015 23:59:11 -0700, <dak@gnu.org> wrote:

>
> https://codereview.appspot.com/249820043/diff/1/lily/simple-spacer.cc
> File lily/simple-spacer.cc (right):
>
>
https://codereview.appspot.com/249820043/diff/1/lily/simple-spacer.cc#newcode199
> lily/simple-spacer.cc:199: programming_error ("misuse of expand_line");
> Any sensible fallback behavior for this "misuse of expand_line"?  If we
> can reliably guess what the intent of the "misuse" is (after all, it
> would appear that the situation comes about from reasonably regular
> input rather than explicit user error), one could just do that and
> forget about the programming error.

These two asserts cannot be triggered in current code, except maybe through
roundoff error, because the only calls of the function are protected my matching
conditionals in Simple_spacer::solve()

The third assert is the one that trips (windows-only).


> Is there something reasonably "continuous" one can do so that small
> errors lead to small offsets?

The following line obviously returns a quantity that changes continuously across
the assert condition,
so I am 90% sure you are defying national stereotypes and using irony.

In that case, yes, I do think the behavior that was being protected against is
so benign that there need be no traps at all for these conditions.

[dak, 2015-06-21 05:40:07]

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

> On Fri, 19 Jun 2015 23:59:11 -0700, <dak@gnu.org> wrote:
>
>>
>> https://codereview.appspot.com/249820043/diff/1/lily/simple-spacer.cc
>> File lily/simple-spacer.cc (right):
>>
>>
https://codereview.appspot.com/249820043/diff/1/lily/simple-spacer.cc#newcode199
>> lily/simple-spacer.cc:199: programming_error ("misuse of expand_line");
>> Any sensible fallback behavior for this "misuse of expand_line"?  If we
>> can reliably guess what the intent of the "misuse" is (after all, it
>> would appear that the situation comes about from reasonably regular
>> input rather than explicit user error), one could just do that and
>> forget about the programming error.
>
> These two asserts cannot be triggered in current code, except maybe
> through roundoff error, because the only calls of the function are
> protected my matching conditionals in Simple_spacer::solve()
>
> The third assert is the one that trips (windows-only).
>
>
>> Is there something reasonably "continuous" one can do so that small
>> errors lead to small offsets?
>
> The following line obviously returns a quantity that changes
> continuously across the assert condition,
> so I am 90% sure you are defying national stereotypes and using irony.

I'll take the 10% option where I did not actually look at the code with
enough detail to see that.

> In that case, yes, I do think the behavior that was being protected
> against is so benign that there need be no traps at all for these
> conditions.

Even a slight aberration can ruin a subsequent loop relying on sorted
input.  I have not looked at the following code enough to see whether we
have that kind of situation relying on monotonicity here.

If we don't, ditching the warning seems like the easiest course.

-- 
David Kastrup

[Keith, 2015-06-24 04:47:22]

https://codereview.appspot.com/249820043/diff/1/lily/simple-spacer.cc
File lily/simple-spacer.cc (right):

https://codereview.appspot.com/249820043/diff/1/lily/simple-spacer.cc#newcode175
lily/simple-spacer.cc:175: line_len_ = line_len;
The logic below is would prove that the first two current assert()s cannot trip
(except in case of roundoff error) unless someone changes this code.

https://codereview.appspot.com/249820043/diff/1/lily/simple-spacer.cc#newcode199
lily/simple-spacer.cc:199: programming_error ("misuse of expand_line");
> Even a slight aberration can ruin a subsequent loop relying on sorted
> input.  I have not looked at the following code enough to see whether we
> have that kind of situation relying on monotonicity here.
> 

There is no sorting of objects based on this output.  This is the code that
figures the fraction of stretchability to use to make a line fit the page
horizontally.
If the calling code above were changed, and expand_line() misused, it would
return slightly too small the 'force' needed to compress the compressible space
to make a line fit, and the line would jut past the right margin.
So reporting a violation of preconditions using a programming error is
reasonable.

https://codereview.appspot.com/249820043/diff/1/lily/simple-spacer.cc#newcode223
lily/simple-spacer.cc:223: if (line_len_ > (1 + 1e-6) * line_len_)
This is, of course,  (line_len_ > (1 + 1e-6) * cur_len)
on my copy. The little man who lives in the server must have made a typo.

[Keith, 2015-06-24 05:35:01]

de-typo and tabs/spaces