dfaure requested changes to this revision. dfaure added inline comments. This revision now requires changes to proceed.
INLINE COMMENTS > sharefd.cpp:32 > + > +//borrowed from klocalsocket.cpp > +class KSockaddrUn Either move it to a common header (if QString everywhere is OK), or use a different classname here, to avoid a classname clash at runtime (and document that it was ported to std::string, so we know why it was forked) > sharefd.cpp:67 > + > +class KMsgHdr > +{ Can you make the classname more descriptive? ShareFDMessageHeader maybe? >From the looks of it I initially thought this came from kio and not just from >sharefd.* > sharefd.cpp:95 > + > +// File descriptor reciever > +FdReceiver::FdReceiver(const QString &path, QObject *parent) typo: receiver > sharefd.cpp:147 > +{ > + if (m_readNotifier) { > + delete m_readNotifier; this if() serves no purpose (delete nullptr is a well-defined no-op) > sharefd.cpp:159 > + > +// File descriptor sender > +FdSender::FdSender(const std::string &path) If FDSender is only used in the kauthhelper, and FDReceiver is only used in kio_file, why not split up this .cpp into two, and only compile the bit that's needed by each target? (you'll need a private header for the shared classes of course, i.e. the sockaddr wrapper and the messageheader would go into sharefd_p.h) It would also make the QString vs std::string API difference look less weird. And then the files can match the classnames, too (fdsender.cpp/h and fdreceiver.cpp/h) > sharefd.cpp:166 > + if (m_socketDes == -1) { > + std::cerr << "socket error:" << strerror(errno); > + return; you need std::endl when using std::cerr (repeats) REVISION DETAIL https://phabricator.kde.org/D6709 To: chinmoyr, thiago, #frameworks, dfaure Cc: dfaure, davidedmundson, elvisangelaccio, shortstheory