D10742: get rid of the raw KFileItem pointers in KCoreDirListerCache

[David Faure]

dfaure requested changes to this revision.
dfaure added a comment.
This revision now requires changes to proceed.


  Since the goal is to speed up findByUrl, can you add a Q_BENCHMARK for findByUrl?
  I do expect it to be faster of course --- a little bit at the expense of the code which modifies items and now has to reinsert them (and that code is hard to benchmark, it's mostly called from async slots...).
  
  Thanks.

INLINE COMMENTS

> kcoredirlister.cpp:2463
>          Q_ASSERT(!item.isNull());
> -        (*lstNewItems)[directoryUrl].append(item);              // items not filtered
> +        (lstNewItems)[directoryUrl].append(item);              // items not filtered
>      } else {

The parenthesis around lstNewItems can be removed now.

> kcoredirlister.cpp:2476
> +    auto kit = items.cbegin();
> +    const auto kend = items.end();
>      for (; kit != kend; ++kit) {

end() inconsistent with cbegin() on the line just above.

Works because the container is const, but still, better be consistent.

> kcoredirlister.cpp:2489
>              Q_ASSERT(!item.isNull());
> -            (*lstNewItems)[directoryUrl].append(item);
> +            (lstNewItems)[directoryUrl].append(item);
>          } else {

The parenthesis around lstNewItems can be removed now.

> kcoredirlister.cpp:2691
> +        foreach (const KFileItem &item, *allItems) {
> +            result.append(item);
> +        }

Is this loop necessary? Can't we return KFileItemList(*allItems) ? There's such a ctor in KFileItemList.

> kcoredirlister_p.h:308
> +            auto it = std::lower_bound(dirItem->lstItems.begin(), dirItem->lstItems.end(), oldUrl);
> +            dirItem->lstItems.erase(it);
> +            dirItem->insert(item);

This is invalid code in case lower_bound() returns end().

If you're sure it "shouldn't happen" because reinsert is always called for items that are already in lstItems, then add a Q_ASSERT. Otherwise if().

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D10742

To: jtamate, #frameworks, dfaure
Cc: bruns, kde-frameworks-devel, mwolff, markg, michaelh, ngraham
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20180706/acab453b/attachment-0001.html>