[squid-dev] [PATCH] suffix parsing and skipping

[Amos Jeffries]

On 24/07/2015 4:45 a.m., Kinkie wrote:
> On Thu, Jul 23, 2015 at 3:55 PM, Alex Rousskov <
> rousskov at measurement-factory.com> wrote:
> 
>> On 07/23/2015 01:46 AM, Kinkie wrote:
>>
>>> the only thing I don't understand is the XXX in
>>> parsedSize(); it's not clear why the comment,
>>
>> The parsedSize() method is broken: It claims to return the number of
>> bytes parsed, but it does not return that. It usually returns the number
>> of bytes parsed from the left, but not from the right of the string. For
>> example,
>>
>>   skipOne()
>>   skipOneTrailing()
>>
>> should usually result in parsedSize() returning 2 because 2 bytes were
>> parsed and removed from the string. It usually returns 1.
>>
>>
>>> and what to use instead.
>>
>> There is currently no good alternative. Discussing the best way forward
>> is outside this project scope (but I welcome such a separate discussion,
>> of course). Fortunately, no existing code misuses parsedSize() today AFAIK.
>>
>>
>>> as soon as that's improved, +1.
>>
>> Why?! This is not something the patch breaks. Do you want me to remove
>> the XXX and just pretend that we have not discovered the problem?
>>
> 
> Amos has brought some objections to the proposal, I'll try to clarify what
> I meant:
> because the comment is issuing guidance to callers of that API: as
> proposed, the guidance is incomplete both for any callers trying to
> understand what API to use.
> If anything, there should be a BUG mark so that implementors may fix it.
> 

Like I said 3/4 of the problem are new code paths introducing the old
bug behaviour as gospel. Thats not a good way forward.

Particular since the full fix is a one-liner in consumeTrailing() in
place of that comment, and currently affecting no other code that would
need auditing.
That one-liner being identical to the matching code in consume():
    parsed_ += result.length();

Amos