----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4992/#review7639 -----------------------------------------------------------
Picky little comment follows! /trunk/qpid/cpp/src/qpid/sys/posix/SystemInfo.cpp <https://reviews.apache.org/r/4992/#comment16849> Why not rename FreeAddrInfo as AddrInfo or perhaps AddrInfoHolder (I don't like naming an object with a verb) and have it also proxy the addrinfo struct as well as do the RAII: viz ... AddrInfo result; int error = ::getaddrinfo(host.c_str(), NULL, NULL, &result); if (error) return false; for (struct addrinfo *res = result; res != NULL; res = res->ai_next) { ... and adding an operator addrinfo*&() to the new AddrInfo? You probably also need to add if(ai) ... to the destructor as it's not documented what freeaddinfo does if the addrinfo struct pointer is zero Something like (untested): struct AddrInfo { ::addrinfo* ai; AddrInfo() : ai(0) {} operator ::addrinfo*&() {return ai;} ~AddrInfo() { if (ai) ::freeaddrinfo(ai); } }; This would make the code just a touch more comprehensible IMO [Note also the the new RAII class really can't be copied, but could have move semantics (in C++11)] - Andrew On 2012-05-07 14:34:06, Alan Conway wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/4992/ > ----------------------------------------------------------- > > (Updated 2012-05-07 14:34:06) > > > Review request for qpid, Andrew Stitcher and Steve Huston. > > > Summary > ------- > > Need by the new HA work to avoid connections to self. > - needs windows implementation > - doesn't cover ipv6, neither does other Url handling code at this point > > > Diffs > ----- > > /trunk/qpid/cpp/src/qpid/sys/windows/SystemInfo.cpp 1335016 > /trunk/qpid/cpp/src/tests/CMakeLists.txt 1335016 > /trunk/qpid/cpp/src/tests/Makefile.am 1335016 > /trunk/qpid/cpp/src/tests/SystemInfo.cpp PRE-CREATION > /trunk/qpid/cpp/src/qpid/sys/posix/SystemInfo.cpp 1335016 > /trunk/qpid/cpp/include/qpid/sys/SystemInfo.h 1335016 > > Diff: https://reviews.apache.org/r/4992/diff > > > Testing > ------- > > New unit test/SystemInfo.cpp. > make check passes > > > Thanks, > > Alan > >
