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

[Alex Rousskov]

On 07/23/2015 10:45 AM, Kinkie wrote:
> On Thu, Jul 23, 2015 at 3:55 PM, Alex Rousskov 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?
> 
> 
> 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.

The patch says "XXX: avoid when parsing suffixes/trailing bytes". That
is as complete instruction as a short comment can deliver IMO. There is
no better API today. A curious or confused reader can always get more
information from the commit message that added the XXX (which is also
provided in the posted patch).

Again, parsed size tracking is not something this patch broke. I just
noticed an old problem and marked it.


> If anything, there should be a BUG mark so that implementors may fix it.

Which is what "XXX" is IMO -- a marking of a bug.


I will just remove that XXX comment from the patch. It is outside the
patch scope. Feel free to add a better XXX, BUG, or whatever comment, of
course.


Thank you,

Alex.