----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/14133/#review26090 -----------------------------------------------------------
lib/cpp/src/thrift/transport/TServerSocket.cpp <https://reviews.apache.org/r/14133/#comment50917> Some funky white space here. I think there's a tab mixed in with the spaces. lib/cpp/src/thrift/util/TResourceHolder.h <https://reviews.apache.org/r/14133/#comment50918> This gets the job done, but I think it is overgeneralizing things, and the generalization makes it more painful to use. I think it would be reasonable to tie this class specifically to addrinfo. Then you don't need to template anything, clients don't need to pass in a FREE_T, and your class won't need a resourceIsValid_ bool, it can just check for NULL. lib/cpp/src/thrift/util/TSocketHelper.h <https://reviews.apache.org/r/14133/#comment50919> The thrift codebase recently switched to using < > instead of " " for pulling in thrift headers, because config.h was causing such pain. This should be < >. lib/cpp/src/thrift/util/TSocketHelper.cpp <https://reviews.apache.org/r/14133/#comment50920> < > lib/cpp/src/thrift/util/TSocketHelper.cpp <https://reviews.apache.org/r/14133/#comment50921> spaces here instead of tabs lib/cpp/test/TServerSocketTest.cpp <https://reviews.apache.org/r/14133/#comment50923> Thank you for considering testing for your change. But... I don't think this exercises your code changes. I don't think it adds much coverage over the existing transport tests. I'm fine with reverting this file and the associated test/Makefile.am. lib/cpp/test/TServerSocketTest.cpp <https://reviews.apache.org/r/14133/#comment50922> Most thrift tests use port 9090. Not a showstopper though. - Ben Craig On Sept. 13, 2013, 6:57 p.m., Ben Craig wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/14133/ > ----------------------------------------------------------- > > (Updated Sept. 13, 2013, 6:57 p.m.) > > > Review request for Thrift. > > > Bugs: THRIFT-1201 > https://issues.apache.org/jira/browse/THRIFT-1201 > > > Repository: Thrift > > > Description > ------- > > THRIFT-1201: getaddrinfo resource leak > Client: cpp > Patch: Frank Meerkoetter > > > Diffs > ----- > > lib/cpp/Makefile.am 7b879f3 > lib/cpp/src/thrift/server/TNonblockingServer.cpp b9553c4 > lib/cpp/src/thrift/transport/TServerSocket.cpp 59a0885 > lib/cpp/src/thrift/util/TResourceHolder.h PRE-CREATION > lib/cpp/src/thrift/util/TSocketHelper.h PRE-CREATION > lib/cpp/src/thrift/util/TSocketHelper.cpp PRE-CREATION > lib/cpp/test/Makefile.am d04cfb5 > lib/cpp/test/TServerSocketTest.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/14133/diff/ > > > Testing > ------- > > Windows testing in progress > > > Thanks, > > Ben Craig > >