Review Request: Adding "net usershare" suport for KSambaShare

[David Faure]

-----------------------------------------------------------
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
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20101115/bcbe5214/attachment.htm>