Re: [PATCH] String API re-implementation using SBuf

On 23/11/2013 5:35 a.m., Alex Rousskov wrote:
> On 11/22/2013 08:25 AM, Amos Jeffries wrote:
>> I've gone through the non-String code use of String::defined() and tried
>> to find other ways of avoiding the defined()/NULL testing for ambiguous
>> cases of String/SBuf differences.
>
>
>> - } else if (rep->cache_control->hasNoCache() && !rep->cache_control->noCache().defined() && !REFRESH_OVERRIDE(ignore_must_revalidate)) {
>> + } else if (rep->cache_control->hasNoCache() && rep->cache_control->noCache().size() > 0 && !REFRESH_OVERRIDE(ignore_must_revalidate)) {
>
> Bug. If you replace defined() with size(), this bug will not happen
> because !defined() is going to be correctly replaced with !size().
>
>
>> - httpHeaderParseQuotedString(tmp.termedBuf(), tmp.size(), &entry.detail);
>> + int detailsParseOk = httpHeaderParseQuotedString(tmp.termedBuf(), tmp.size(), &entry.detail);
>> tmp = parser.getByName("descr");
>> - httpHeaderParseQuotedString(tmp.termedBuf(), tmp.size(), &entry.descr);
>> - bool parseOK = entry.descr.defined() && entry.detail.defined();
>> + int descrParseOk = httpHeaderParseQuotedString(tmp.termedBuf(), tmp.size(), &entry.descr);
>
> Please mark the new locals constant.
>
>
> Throughout the patch please use size() instead of size()>0 if at all
> possible, especially in the code that already uses succinct expressions
> in this and/or other contexts. I cannot insist on that, of course, but I
> have to ask.
>
> I did not find any other problems.
>
>
> Thank you,
>
> Alex.
>

Gah. This email got delayed and I comitted after you earlier/later one
just saying done. Oh well, fixed the bugs here anyway.

Amos
Received on Sat Nov 23 2013 - 11:28:23 MST