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
