Review Request 111870: KDirWatch, make a little bit better use of inotify and prevent a stat call

[David Faure]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/111870/#review37170
-----------------------------------------------------------


Good catch. Just a few suggestions for improvement.


kdecore/io/kdirwatch.cpp
<http://git.reviewboard.kde.org/r/111870/#comment27499>

    You can remove the "? true : false" bit.



kdecore/io/kdirwatch.cpp
<http://git.reviewboard.kde.org/r/111870/#comment27500>

    The new code contradicts this comment ("we'll just take both kinds of clients").
    
    Sounds like the comment needs to be removed, actually, it refers to the stat() call that you removed.



kdecore/io/kdirwatch.cpp
<http://git.reviewboard.kde.org/r/111870/#comment27501>

    So tpath is unused, and isDir is an input parameter instead of an output parameter... this makes for a pretty weird API. How about moving this to a different method instead, and calling that other method only from the inotify code? That would be much more readable than trying to fit a square peg into a round hole :)


- David Faure


On Aug. 5, 2013, 8:17 p.m., Mark Gaiser wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/111870/
> -----------------------------------------------------------
> 
> (Updated Aug. 5, 2013, 8:17 p.m.)
> 
> 
> Review request for kdelibs and David Faure.
> 
> 
> Description
> -------
> 
> At first i wasn't intending to even ask a reviewrequest for this because i want to fix an issue that is well described in my thread: "KDirWatch bug and the analysis. Help is welcome!" on kde-core-devel. However, while starting on fixing that issue i noticed that this would be a nice small improvement to have. It prevents one stat call that was used to determine if "entry" is a file or folder. Inotify already gives us that information under the IN_ISDIR flag thus i use that instead of the stat call.
> 
> 
> Diffs
> -----
> 
>   kdecore/io/kdirwatch.cpp 95ee3d3 
> 
> Diff: http://git.reviewboard.kde.org/r/111870/diff/
> 
> 
> Testing
> -------
> 
> Compiles just fine.
> Runs fine, i haven't seen any issues thus far.
> Oh and it passes all kio test cases but i specially note the following:
> kdirwatchtest
> kdirlistertest
> kdirmodeltest
> 
> 
> Thanks,
> 
> Mark Gaiser
> 
>

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