On 14/04/2014 6:20 a.m., Alex Rousskov wrote: > On 04/10/2014 11:32 PM, Amos Jeffries wrote: >> I believe the attached patch is complete regarding the method conversion. > > The patch looks OK to me. The comments below are minor and can be > ignored or fixed during commit. > > >> + sb.append(s.rawContent(), s.length()); >> + sb.append(s.rawContent(), s.length()); > ... > > There are quite a few of such calls in the patch and they often require > declaring a temporary SBuf variable. Consider adding > MemBuf::append(const SBuf &sb) method to handle these better. >
5 in total converting to both MemBuf and String types. The plan is to remove those MemBuf and String objects being appended into soon in the SBuf transition. > >> Log output quoting was what you meant. > >> + debugs(11, 2, "Request not yet fully sent \"" << request->method << >> ' ' << entry->url() << '\"' ); > >> + debugs(11, 3, '\"' << fwd->request->method << ' ' << fwd->entry->url() >> << '\"'); > > > The above two changed lines still have unneeded (IMO) "log output quoting". > Oops. Fixed. > > >> + logfilePrintf(logfile, "%s %s %s [%s] \"" SQUIDSBUFPH " %s %s/%d.%d\" >> %d %" PRId64 " \"%s\" \"%s\" %s%s:%s%s", >> clientip, >> user_ident ? user_ident : dash_str, >> user_auth ? user_auth : dash_str, >> Time::FormatHttpd(squid_curtime), >> - al->_private.method_str, >> + SQUIDSBUFPRINT(method), > > I think it is only a matter of time when Time::FormatHttpd() or another > function used in this or similar context will call SQUIDSBUFPRINT() to > format something internally. This problem is out of your patch scope, of > course, but we should move towards stream << printing to avoid such > problems and resist adding more printfs. > Agreed. One last build test and this will be applied later today. Amos
