On 12/05/2013 8:59 a.m., Alex Rousskov wrote:
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.
* 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.
That function was already the only public one in the class, therefore it
seems the XXX todo entry was already implemented.
Also, the callers you seem to be refering to should be only the main
logfile API for agnostice functions. The function you marked is the
method for that API to use to trigger TcpLoggers existence.
* 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.
No it was for us. I found it hard to even see the second definition and
took it as a single block of documentation for a bit.
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).
Ouch. right.
* 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.
Okay. Fine.
Thank you guys.
Amos
