----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/4320/#review8750 -----------------------------------------------------------
/trunk/KDE/kdelibs/kio/kio/ksambashare.h <http://svn.reviewboard.kde.org/r/4320/#comment9339> Documentation? Ownership of the returned pointer? ==> another reason to return a value instead. /trunk/KDE/kdelibs/kio/kio/ksambashare.h <http://svn.reviewboard.kde.org/r/4320/#comment9340> Why did you move away from Q_PRIVATE_SLOT? This looks like a step in the wrong direction. /trunk/KDE/kdelibs/kio/kio/ksambashare.cpp <http://svn.reviewboard.kde.org/r/4320/#comment9333> Please keep the original solution of static const char* const ... []. It made the data really-const (.rodata section), i.e. shared between processes and not relocated in memory. /trunk/KDE/kdelibs/kio/kio/ksambashare.cpp <http://svn.reviewboard.kde.org/r/4320/#comment9334> To keep the C++ code maintainable and modular, please declare and use variables in the same line, e.g. QString rawString = testparmParamValue(...); Then you can even make it const to indicate that it's not modified anywhere in the method. const QString rawString = testparmParamValue(...); ("parmParam? That's a bit difficult to read, don't both words mean "parameter"?) /trunk/KDE/kdelibs/kio/kio/ksambashare.cpp <http://svn.reviewboard.kde.org/r/4320/#comment9337> It seems to me that this "new KSambaShareData" instance is leaked, in the destructor. Why not use this by value, given that you made it a nice value-based class? In this case the code would look like KSambaShareData& shareData = data[currentShare]; // find or insert shareData.setName(currentShare); ... /trunk/KDE/kdelibs/kio/kio/ksambashare.cpp <http://svn.reviewboard.kde.org/r/4320/#comment9335> const QString key = ... const QString value = ... - David On 2010-11-14 22:48:05, Rodrigo Belem wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://svn.reviewboard.kde.org/r/4320/ > ----------------------------------------------------------- > > (Updated 2010-11-14 22:48:05) > > > Review request for kdelibs, Raphael Kubo da Costa, Jonathan Thomas, Aurélien > Gâteau, Jonathan Riddell, Adenilson Cavalcanti, loureiro, and Daniel > Nicoletti. > > > Summary > ------- > > KDE needs to support modern samba tools. With the "net usershare" command > line tool the users can manage their shares. The attached patch intends to > add support for this tool. > > > Diffs > ----- > > /trunk/KDE/kdelibs/includes/CMakeLists.txt 1180108 > /trunk/KDE/kdelibs/kio/CMakeLists.txt 1180108 > /trunk/KDE/kdelibs/kio/kio/ksambashare.h 1180108 > /trunk/KDE/kdelibs/kio/kio/ksambashare.cpp 1180108 > /trunk/KDE/kdelibs/kio/kio/ksambashare_p.h PRE-CREATION > /trunk/KDE/kdelibs/kio/kio/ksambasharedata.h PRE-CREATION > /trunk/KDE/kdelibs/kio/kio/ksambasharedata.cpp PRE-CREATION > /trunk/KDE/kdelibs/kio/kio/ksambasharedata_p.h PRE-CREATION > > Diff: http://svn.reviewboard.kde.org/r/4320/diff > > > Testing > ------- > > > Thanks, > > Rodrigo > >