From: Damiano B. <da...@da...> - 2012-06-20 14:15:07
|
On Wed, 2012-06-20 at 15:44 +0200, Max Kellermann wrote: > PLEASE keep the mailing list in Cc! > > On 2012/06/20 15:32, Damiano Bortolato <da...@da...> wrote: > > https://github.com/damianob/xcsoar.git > > Rebase on official master branch before you push. > > Patch should be split. One patch that adds generic support for UDP to > SocketDescriptor, one patch that adds a UDP "Port" implementation, ... > Ok > Wrong switch/case indentation in ConfiguredPort.cpp and > DeviceConfig.cpp. > Ok > Uninitialised network_protocol attribute in DeviceConfig::Clear(). > > Duplicate code in SocketDescriptor. And the same amount of duplicate > code in UDPPort. You did not write 99% of the code, you merely > changed the socket type (STREAM to DGRAM). > I know, so what do you suggest? Should I write a generic CreateListener(unsigned port, unsigned backlog, DeviceConfig::NetworkProtocolType) ? Should I derive TCPPort and UDPPort from a generic NetworkPort? > You renamed "TCP_LISTENER" to "NETWORK", however you dropped the > "LISTENER" part from the name. There's a reason I put it there. > I'll put it back > Use doxygen style API documentation in SilentWings.cpp. Avoid long > lines if you can. > Ok > Why include Device/Parser.hpp? > I've Forgotten it there. > The printf() should be removed, and other obvious debug code, > including comments with questions. > > The SilentWingsBinary struct cannot be read safely, due to misaligned > "double" attributes. Your code is not portable, and that is even > without considering the byte ordering and floating point > representation issues. What else is making it non portable? Damiano |