D10742: get rid of the raw KFileItem pointers in KCoreDirListerCache

2019-07-21 Thread Jaime Torres Amate
jtamate closed this revision. jtamate added a comment. In D10742#499346 , @aacid wrote: > @jtamate Does that "let's continue in that other review" mean we should cancel this one? Still shows on the list of reviews to consider Yes, it

D10742: get rid of the raw KFileItem pointers in KCoreDirListerCache

2019-07-21 Thread Albert Astals Cid
aacid added a comment. @jtamate Does that "let's continue in that other review" mean we should cancel this one? Still shows on the list of reviews to consider REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D10742 To: jtamate, #frameworks, dfaure Cc: aacid,

D10742: get rid of the raw KFileItem pointers in KCoreDirListerCache

2018-12-16 Thread Jaime Torres Amate
jtamate added a comment. Let's continue on D17619 REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D10742 To: jtamate, #frameworks, dfaure Cc: elvisangelaccio, bruns, kde-frameworks-devel, mwolff, markg, michaelh, ngraham

D10742: get rid of the raw KFileItem pointers in KCoreDirListerCache

2018-12-15 Thread Elvis Angelaccio
elvisangelaccio added a comment. In D10742#377356 , @jtamate wrote: > Let's see if this assert/crash can be avoided without reverting all the patch. Any help is welcome. Does this valgrind log help? Invalid read of size 8

D10742: get rid of the raw KFileItem pointers in KCoreDirListerCache

2018-12-15 Thread Jaime Torres Amate
jtamate added a comment. I'm able to reproduce the bug with the following steps: - Create a folder structure X/X1/X2/X3/X4 and add a new empty text file into each folder. - Within Dolphin, open a tab for each folder. - In the tab showing X contents, rename X1 to X1_ and the crash is

D10742: get rid of the raw KFileItem pointers in KCoreDirListerCache

2018-12-15 Thread Jaime Torres Amate
jtamate added a comment. I've been able to reproduce the bug: #10 0x7f44d257ae1d in qt_assert (assertion=assertion@entry=0x7f44d456fc43 "it != dirItem->lstItems.end()", file=file@entry=0x7f44d456fc10 "/g/5kde/frameworks/kio/src/core/kcoredirlister_p.h", line=line@entry=308) at

D10742: get rid of the raw KFileItem pointers in KCoreDirListerCache

2018-12-14 Thread Elvis Angelaccio
elvisangelaccio reopened this revision. elvisangelaccio added inline comments. This revision is now accepted and ready to land. INLINE COMMENTS > kcoredirlister.cpp:1576-1577 > +for (auto kit = dir->lstItems.begin(), kend = > dir->lstItems.end(); kit != kend; ++kit) { >

D10742: get rid of the raw KFileItem pointers in KCoreDirListerCache

2018-10-09 Thread Jaime Torres Amate
This revision was automatically updated to reflect the committed changes. Closed by commit R241:20b89972b643: get rid of the raw KFileItem pointers in KCoreDirListerCache (authored by jtamate). REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D10742?vs=42345=43240

D10742: get rid of the raw KFileItem pointers in KCoreDirListerCache

2018-10-03 Thread David Faure
dfaure accepted this revision. dfaure added a comment. This revision is now accepted and ready to land. I think I'm fine with it now, but please wait until next Monday (Oct 8) before pushing, so it doesn't break the upcoming KF5 release. I'm not 100% confident (given that earlier

D10742: get rid of the raw KFileItem pointers in KCoreDirListerCache

2018-09-26 Thread Jaime Torres Amate
jtamate updated this revision to Diff 42345. jtamate marked 5 inline comments as done. jtamate edited the summary of this revision. jtamate edited the test plan for this revision. jtamate added a comment. I've tried to do an automatic benchmark of the rename case, without success. Changed

D10742: get rid of the raw KFileItem pointers in KCoreDirListerCache

2018-07-06 Thread 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

D10742: get rid of the raw KFileItem pointers in KCoreDirListerCache

2018-06-01 Thread Jaime Torres Amate
jtamate updated this revision to Diff 35305. jtamate marked 4 inline comments as done. jtamate edited the summary of this revision. jtamate edited the test plan for this revision. jtamate added a comment. The change in kdirmodel is not needed anymore. The methods that were const are const

D10742: get rid of the raw KFileItem pointers in KCoreDirListerCache

2018-05-30 Thread Jaime Torres Amate
jtamate added a dependency: D13211: Enable comparing KFileItems by url. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D10742 To: jtamate, #frameworks, dfaure Cc: bruns, kde-frameworks-devel, mwolff, markg, michaelh, ngraham

D10742: get rid of the raw KFileItem pointers in KCoreDirListerCache

2018-05-28 Thread David Faure
dfaure requested changes to this revision. dfaure added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > kcoredirlister.cpp:963 > QUrl oldurl = src.adjusted(QUrl::StripTrailingSlash); > -KFileItem *fileitem = findByUrl(nullptr, oldurl); > -if

D10742: get rid of the raw KFileItem pointers in KCoreDirListerCache

2018-05-24 Thread Jaime Torres Amate
jtamate updated this revision to Diff 34775. jtamate marked 5 inline comments as done. jtamate edited the summary of this revision. jtamate added a comment. Removed m_hash, after implementing the right checks it was slower than comparing QUrls. REPOSITORY R241 KIO CHANGES SINCE LAST

D10742: get rid of the raw KFileItem pointers in KCoreDirListerCache

2018-05-23 Thread Stefan Brüns
bruns added inline comments. INLINE COMMENTS > kcoredirlister_p.h:302 > +// Remove the item from the sorted list (from the wrong place) and > insert it in the right place. > +void reinsert(KFileItem , const QUrl ) > +{ This can be better implemented with std::move and

D10742: get rid of the raw KFileItem pointers in KCoreDirListerCache

2018-05-23 Thread Stefan Brüns
bruns added inline comments. INLINE COMMENTS > kfileitem.cpp:1248 > +{ > +return d->m_hash < other.d->m_hash; > +} This is incomplete for two cases: 1. Same URL 2. Hash collision > kfileitem.h:490 > +/** > + * Returns -1 if other's URL is greater, 0 if == and 1 if less than (as >

D10742: get rid of the raw KFileItem pointers in KCoreDirListerCache

2018-05-23 Thread Jaime Torres Amate
jtamate updated this revision to Diff 34708. jtamate edited the summary of this revision. jtamate edited the test plan for this revision. jtamate added a comment. Restricted Application added a subscriber: kde-frameworks-devel. Based on the tests done in D12945

D10742: get rid of the raw KFileItem pointers in KCoreDirListerCache

2018-03-19 Thread Jaime Torres Amate
jtamate planned changes to this revision. jtamate added a comment. I've noticed this creates/exposes some crashes in other code/tests, for example in kdirmodeltest the second and later times the test is run. I'll check all the failing tests (given enough time). REPOSITORY R241 KIO

D10742: get rid of the raw KFileItem pointers in KCoreDirListerCache

2018-03-15 Thread Milian Wolff
mwolff added a comment. no real review, just want to mention that you'll still have heap allocations for every item - now once per container it is in (due to QList and QSet and sizeof the KFileItem) REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D10742 To: jtamate,

D10742: get rid of the raw KFileItem pointers in KCoreDirListerCache

2018-03-15 Thread Jaime Torres Amate
jtamate updated this revision to Diff 29572. jtamate edited the summary of this revision. jtamate edited the test plan for this revision. jtamate added a comment. Pass all the unittests, without modifications, finally. Use as much as possible the recently added move semantics in KFileItem.

D10742: get rid of the raw KFileItem pointers in KCoreDirListerCache

2018-03-05 Thread Jaime Torres Amate
jtamate added a comment. In D10742#218222 , @markg wrote: > In D10742#218171 , @dfaure wrote: > > > Can someone explain to me how switching from pointers to values is making anything faster, or is a

D10742: get rid of the raw KFileItem pointers in KCoreDirListerCache

2018-03-04 Thread Mark Gaiser
markg added a comment. In D10742#218171 , @dfaure wrote: > Can someone explain to me how switching from pointers to values is making anything faster, or is a first step towards making anything faster? This step in itself can only make things

D10742: get rid of the raw KFileItem pointers in KCoreDirListerCache

2018-03-04 Thread David Faure
dfaure added a comment. Can someone explain to me how switching from pointers to values is making anything faster, or is a first step towards making anything faster? This step in itself can only make things slower due to more "copying" (refcount updating). REPOSITORY R241 KIO REVISION

D10742: get rid of the raw KFileItem pointers in KCoreDirListerCache

2018-02-24 Thread Jaime Torres Amate
jtamate updated this revision to Diff 27909. jtamate edited the test plan for this revision. jtamate added a comment. The first patch had 12 unit test failing. This one has 2 1/2 unit test failing. I've changed findByUrl, instead of removing the item from the list, tell the item to

D10742: get rid of the raw KFileItem pointers in KCoreDirListerCache

2018-02-22 Thread Mark Gaiser
markg added a comment. I unfortunately have no clue how to answer your questions. INLINE COMMENTS > kcoredirlister.cpp:825-829 > +// If take remove the element from the list. > +if (take) { > +dirItem->lstItems.erase(it); > +

D10742: get rid of the raw KFileItem pointers in KCoreDirListerCache

2018-02-22 Thread Jaime Torres Amate
jtamate created this revision. jtamate added reviewers: Frameworks, dfaure. Restricted Application added a project: Frameworks. jtamate requested review of this revision. REVISION SUMMARY This summary will change when the doubts are resolved (if they can be resolved!). Implement the first