Review Request 128514: KIconLoader: reduce number of lookups when doing fallbacks

[Anthony Fieroni]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128514/#review97829
-----------------------------------------------------------




src/kiconloader.cpp (line 1037)
<https://git.reviewboard.kde.org/r/128514/#comment65924>

    This is not correct. When you have genericFallback && !currentName.empty() you never reach line 1071 to get an icon. So your code returns an empty path for non empty currentName in genericFallback. If this can be expected, i'm not familliar with path expectations, you can make exclusive check before 2 loops, i.e. before foreach.


- Anthony Fieroni


On Юли 24, 2016, 3:27 след обяд, David Faure wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128514/
> -----------------------------------------------------------
> 
> (Updated Юли 24, 2016, 3:27 след обяд)
> 
> 
> Review request for KDE Frameworks, Aleix Pol Gonzalez and Christoph Feck.
> 
> 
> Repository: kiconthemes
> 
> 
> Description
> -------
> 
> In the first pass, we already checked whether the icon exists in all icon themes.
> So in the second pass, there's no point in starting with that again, we can go
> ahead and start doing fallbacks right away.
> 
> (The comment about the two passes was removed in b84858c, but some of it still
> applies.)
> 
> Tested with: strace -e file ./kiconengine_unittest testUnknownIconNotCached |& wc -l
> Before: 6341
> After: 4589
> 
> 
> Diffs
> -----
> 
>   autotests/kiconloader_benchmark.cpp ded5e0221f8126139a019a8310e8f120672ac3b4 
>   src/kiconloader.cpp 951775d77b7ec0e2d8099ee192ca25321dab4544 
> 
> Diff: https://git.reviewboard.kde.org/r/128514/diff/
> 
> 
> Testing
> -------
> 
> strace as mentionned above.
> 
> All unittests still pass (incl fallback lookups, didn't check how complete those were, though)
> 
> 
> Thanks,
> 
> David Faure
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20160725/710d9a8a/attachment-0001.html>