Deprecate KFileItemList and introduce KFileItemListV2
Needs ReviewPublic

Authored by markg on Feb 8 2018, 12:09 AM.

Details

Reviewers
dfaure
Summary

Summary
Deprecate KFileItemList. It's using QList and lacks move semantics.
Introducing KFileItemListV2 which is just a using statement for std::vector<KFileItem>.
This new list allows for more efficient moving of KFileItem objects using move semantics.

Also added (and immediately deprecated) a "toKFileItemList" function that converta a KFileItemListV2 back to a KFileItemList to keep the public API usable.

Diff Detail

Repository
R241 KIO
Branch
kfileitemlist2
Lint
No Linters Available
Unit
No Unit Test Coverage
markg created this revision.Feb 8 2018, 12:09 AM
Restricted Application added a project: Frameworks. · View Herald TranscriptFeb 8 2018, 12:09 AM
Restricted Application added a subscriber: Frameworks. · View Herald Transcript
markg requested review of this revision.Feb 8 2018, 12:09 AM
dfaure added a comment.Feb 8 2018, 6:57 AM

I would deprecate KFileItemList only when KFileItemVector is a valid replacement (i.e. all the APIs in KIO use KFileItemVector). Deprecating while people are still forced to use it, only leads to a habit of ignoring deprecation signals (since there's no other solution). Introduce Vector, but don't deprecate List just yet.

src/core/kfileitem.h
550

It's not bad, when sizeof(T) == sizeof(void*), which is the case here.

The point about move semantics remains, but that's the only one; let's stick to facts in comments.

553

it's -> its

But why not add those convenience methods again, one way or another? Otherwise we'll have massive code duplication.

591

typo: vector

594

How about naming KFileItemVector instead? Much cleaner name.

markg added a comment.Feb 8 2018, 12:30 PM

Based on the comment you made in D10380 it's logical that this commit won't make it as it currently is implemented.
The using line is probably also not the best option if you want to keep the convenience functions, inheriting would be better. But then again, inheriting from an std container gives it's own issues (like no virtual destructor...)

std::vector, for example, is not designed to be inherited and typically should not be inherited (very shaky design), as that will then be prone to this base pointer deletion issue (std::vector deliberately avoids a virtual destructor) in addition to clumsy object slicing issues if your derived class adds any new state.

See: https://softwareengineering.stackexchange.com/questions/284561/when-not-to-use-virtual-destructors

That kinda leaves me wondering how to proceed with this.
Some options i can think of:

  1. Do nothing. Leave as is for now and the foreseeable time. No move semantics.
  2. Go for the using line (as KFileItemVector) but with a "KFileItemVectorHelper" namespace that contains the current helper function. (or just the KIO:: namespace).
    1. In this case not deprecating the existing API before all other functions that currently use KFileItemList have a KFileItemVector counterpart
    2. Also put this under a compile time define (say -DUSE_KFILEITEMVECTOR) which means the code will be littered with #if statements, but maintains performance as it currently stands. Using this define would disable KFileItemList so that too would be under #if statements.
    3. This also allows for a more graceful transition as not everything has to be implemented at once. Once everything is implemented, add a whole slew of deprecated warnings to all KFileItemList uses.
    4. In KF6 simply remove all #if and end up with only KFileItemVector code paths.
  3. Something else?

What's your opinion?

@dfaure What's your preference here?
I do "want" to work on this, no promises though. It could take a very long while ;)

Yep, option 2 sounds like an incremental plan, which is good.