kossebau abandoned this revision.
kossebau added a comment.
As I am not sure about the risk introduced by the just-works-currently
alignment in the union, I would rather discard this patch for now. Going from
union to having both fields increases the runtime for what I measured, despite
the
dfaure requested changes to this revision.
dfaure added inline comments.
This revision now requires changes to proceed.
INLINE COMMENTS
> udsentry.h:132
> +const long long l;
> +char s[sizeof(QString)];
> +} m_u;
Why not simply QString*?
> udsentrybenchmark.cpp:141
> +
kossebau updated this revision to Diff 79210.
kossebau added a comment.
Update to latest master
REPOSITORY
R241 KIO
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D28528?vs=79182&id=79210
BRANCH
udsentryinitlistconstructor
REVISION DETAIL
https://phabricator.kde.org/D28528
A
apol added a comment.
+1 Makes sense, the code looks a bit fiddly but should allow cleaner code.
INLINE COMMENTS
> kossebau wrote in udsentry.h:127
> Do you mean deep-copy by memcopy? There should be no such, the
> `UDSEntryInitListEntry(uint field, const QString &value)` constructor does a
kossebau added inline comments.
INLINE COMMENTS
> apol wrote in udsentry.h:127
> I'm not sure, it skips the constructor/destructor, but then when it's a
> string we need to do a memcopy. Are you sure this is faster than the
> refcounting?
Do you mean deep-copy by memcopy? There should be no su
apol added inline comments.
INLINE COMMENTS
> udsentry.h:127
> +// holds either the numeric or the string value
> +// done to avoid unneeded QString constructor & destructor in case of
> numeric value
> +union U {
I'm not sure, it skips the constructor/destructor, but then when it's
kossebau created this revision.
kossebau added reviewers: Frameworks, dfaure, apol.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
kossebau requested review of this revision.
REVISION SUMMARY
For entries with fixed set of fields having an initializer list op
