On 24/07/2015 4:45 a.m., Kinkie wrote: > On Thu, Jul 23, 2015 at 3:55 PM, Alex Rousskov < > rouss...@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 _______________________________________________ squid-dev mailing list squid-dev@lists.squid-cache.org http://lists.squid-cache.org/listinfo/squid-dev