D12696: Use the new uds implementation

[David Faure]

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


  Looks good.

INLINE COMMENTS

> udsentry.cpp:45
> +        inline Field(const uint index, long long value = 0) : m_long(value), m_index(index) {}
> +        // This operator helps to gain 1ms just comparing the key
> +        inline bool operator == (const Field &other) const {

1ms is relative to a benchmark which isn't clear when reading this code in other contexts. As someone said in another review, do we still need this operator== anyway, given that you pass lambdas to find_if()?

> udsentry.cpp:55
> +    std::vector<Field> storage;
> +public:
> +    void reserve(int size)

remove this "public:", we're already in the public section.

But actually, now that there are methods wrapping all usage of "storage", how about making "storage" private? (moving it to the end of the class, to respect the standard order in Qt/KF5 code: public / protected / private).

Actually, the Field class definition can move with it too.

Ah, I see a few remaining uses from the outside, how about moving it all in for proper encapsulation? Some save/load methods with QDataStream, and a method that takes QDebug...

> udsentry.cpp:118
> +    }
> +    QList<uint> listFields() const
> +    {

#ifndef KIOCORE_NO_DEPRECATED

like the only caller

> udsentry.cpp:120
> +    {
> +        QList<uint> res;
> +        for (auto it = storage.cbegin(), end = storage.cend(); it != end; ++it) {

res.reserve(storage.count());

> udsentry.cpp:128
> +    {
> +        QVector<uint> res;
> +        for (auto it = storage.cbegin(), end = storage.cend(); it != end; ++it) {

res.reserve(storage.count());

> udsentry.cpp:414
> +        default:
> +            return QString("Unknow uds field %1").arg(field);
> +    }

Typo: unknown

> udsentry.h:320
> +     */
> +    void replaceOrInsert(uint field, const QString &value);
> +

@since 5.47

> udsentry.h:327
> +     */
> +    void replaceOrInsert(uint field, long long l);
> +

@since 5.47

Should these be called just replace() like QMultiMap::replace also means "replace or insert" ?

> udsentry.h:333
> +     */
> +    static QString nameOfUdsField(uint field);
>  };

I see you're using this internally, is it needed in the public API? I'd just make it internal.

REPOSITORY
  R241 KIO

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

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