On 05/11/2013 12:36 AM, Amos Jeffries wrote:
> On 30/04/2013 7:14 a.m., Alex Rousskov wrote:
>> I believe this snapshot addresses all review issues raised so far.
> The rest that I can find is mostly just polishing bits.
>
> in cf.data.pre:
> * text about module"none" has "]]" at the end of the optional ACL list.
> Please take the opportunity to remove one of the brackets.
Done.
> in src/cache_cf.cc:
> * doxygen comment before parse_access_log() is missing the " * " prefix
> to each line signalling that it is a doxygen comment text and not a
> verbatim code example.
Done.
> in src/log/TcpLogger.h
> * please remove the squid.h include. It must be first in the .cc and
> _not_ present in any .h.
Done.
> * please convert the #ifdef to #if for HAVE_LIST
Done.
> * please remove the XXX commenat about Open().
Done. Although the XXX was correct IMO: It is wrong to expose the
callers to all the TcpLogger details when all they need is one
TCP-agnostic function call.
> * please add whitespace line between logRecord() and start of
> documentation for flush(). Same in .cc between the global-static
> variables near the top.
Done. Just curious, does that empty line help doxygen in some way? I
think I was trying to use sequential code lines to keep related things
visually together.
> * TYPO: "unpexted"
Fixed.
> * please shrink the double-empty lines at the end of file to one (same
> in the .cc file above CBDATA_CLASS_INIT)
Done.
> in src/log/TcpLogger.cc:
> * in Log::TcpLogger::connectDone() please use prefix ++ instead of
> suffix in "connectFailures++ % 100 == 0"
Not done because it would hide the most important failure (the first one).
> * in Log::TcpLogger::Open() please use xfree(strAddr) instead of
> safe_free. safe_free() is only useful if the variable needs to be set
> NULL after free (eg, for members or for later logics).
Not done. I think it is better to leave this as is for now because the
local strAddr variable is left accessible after destruction when
GetHostWithPort() succeeds (the common case). It would be even better to
rewrite this code to avoid explicit free, but this is old, working code
unrelated to the patch scope, so we do not have to polish it. If you
insist, I will polish it separately.
> I am happy for the above to go in during merge, without another audit
> cycle. +1.
Committed to trunk as r12805.
Thank you,
Alex.
Received on Sat May 11 2013 - 20:59:42 MDT
This archive was generated by hypermail 2.2.0 : Sun May 12 2013 - 12:00:07 MDT