D28195: Avoid double fetch and temporary hex encoding for NTFS attributes
This revision was automatically updated to reflect the committed changes. Closed by commit R241:56c738dc251b: Avoid double fetch and temporary hex encoding for NTFS attributes (authored by bruns). CHANGED PRIOR TO COMMIT https://phabricator.kde.org/D28195?vs=78203=78221#toc REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28195?vs=78203=78221 REVISION DETAIL https://phabricator.kde.org/D28195 AFFECTED FILES src/ioslaves/file/file_unix.cpp To: bruns, #dolphin, dfaure Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns
D28195: Avoid double fetch and temporary hex encoding for NTFS attributes
dfaure accepted this revision. dfaure added a comment. This revision is now accepted and ready to land. Oh, I see. Not obvious that this is the reason. `for (decltype(length) n = 0; n < length; ++n)` might be more obvious, although maybe also not very common. I'll let you decide which one you prefer, and push. REPOSITORY R241 KIO BRANCH ntfs_hidden REVISION DETAIL https://phabricator.kde.org/D28195 To: bruns, #dolphin, dfaure Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns
D28195: Avoid double fetch and temporary hex encoding for NTFS attributes
bruns added inline comments. INLINE COMMENTS > dfaure wrote in file_unix.cpp:533 > Given that the value of `n` isn't used, any reason why this doesn't iterate > upwards instead, as the old code was doing? I just find it more usual and > readable. To make sure n and length have the same type. Otherwise an int and a ssize_t, or whatever getxattr* return, are compared. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D28195 To: bruns, #dolphin, dfaure Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns
D28195: Avoid double fetch and temporary hex encoding for NTFS attributes
dfaure added a comment.
Indeed the hex roundtrip was very unnecessary, well spotted.
INLINE COMMENTS
> file_unix.cpp:533
> char *c = strAttr;
> -char *e = hexAttr.data();
> -*e++ ='0';
> -*e++ = 'x';
> -for (auto n = 0; n < length; n++, c++) {
> -*e++ = digits[(static_cast(*c) >> 4)];
> -*e++ = digits[(static_cast(*c) & 0x0F)];
> +for (auto n = length; n > 0; --n, c++) {
> +intAttr <<= 8;
Given that the value of `n` isn't used, any reason why this doesn't iterate
upwards instead, as the old code was doing? I just find it more usual and
readable.
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D28195
To: bruns, #dolphin, dfaure
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns
D28195: Avoid double fetch and temporary hex encoding for NTFS attributes
bruns created this revision. bruns added reviewers: Dolphin, dfaure. Herald added a project: Frameworks. Herald edited subscribers, added: kde-frameworks-devel; removed: Frameworks. bruns requested review of this revision. REVISION SUMMARY The attrib is a DWORD (32 bit unsigned int) in the Windows APIs (see WIN32_FILE_ATTRIBUTE_DATA), and exported as a 4 byte array by ntfs-3g. As the size is known, there is no need to query it. As each file has the "archive" flag set on creation, i.e. the first getxattr call typically never returns 0, this cuts the number of syscalls by half. Skip the temporary hex encoding of the value, it is pointless to hex- encode the value and immediately after parse it again. TEST PLAN 1. touch foo 2. getfattr -n system.ntfs_attrib_be -e hex foo 3. dolphin ./ 4. setfattr -n system.ntfs_attrib_be -v 0x0022 5. refresh dolphin REPOSITORY R241 KIO BRANCH ntfs_hidden REVISION DETAIL https://phabricator.kde.org/D28195 AFFECTED FILES src/ioslaves/file/file_unix.cpp To: bruns, #dolphin, dfaure Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns
