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

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.
Received on Fri Nov 22 2013 - 16:35:29 MST