Use this QFlag to minimize fields retrieved by statx, yielding a little memory and performance yield.
Details
- Reviewers
dfaure kossebau - Group Reviewers
Frameworks - Commits
- R241:9537bca2b542: [StatJob] Use A QFlag to specify the details returned by StatJob
ctest
Diff Detail
- Repository
- R241 KIO
- Branch
- arcpatch-D25010_1
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 19959 Build 19977: arc lint + arc unit
src/core/directorysizejob.cpp | ||
---|---|---|
139–141 | The previous code did (details of 3) |
src/core/directorysizejob.cpp | ||
---|---|---|
139–141 | I'm confused by what you mean here... and I think I found more problems: Indeed the old code used 3. And this means, in the new field statDetails, we want to specify KIO::Inode to get those fields. |
src/core/directorysizejob.cpp | ||
---|---|---|
141 | You want |, not & ... |
src/core/directorysizejob.cpp | ||
---|---|---|
139–141 | This still uses ENABLED instead of BUILD, see initial comment by kossebau. |
Replace use of KIOCORE_ENABLE_DEPRECATED_SINCE by KIOCORE_BUILD_DEPRECATED_SINCE in cpp files
I have tried adding unit tests.
It showed that the old:
*stat(const QUrl &url, KIO::StatJob::StatSide side, short int details, JobFlags flags = DefaultFlags);
Takes over the new :
KIO::stat(const QUrl &, KIO::StatSide, KIO::StatDetails int, JobFlags)")
Since KIO::StatDetails data type is int and enums are transparently converted to integer.
I will need to find a workarount or rename/change the signature of the new KIO::stat to make them apart
Fix implementation having a statDetails function to distinguish from the original function, fix and adapt test, in file_unix when using statx reduce the field retrieved for symlinks
src/core/statjob.cpp | ||
---|---|---|
97 ↗ | (On Diff #71701) | When the declaration is in #if, the definition should be too (with s/ENABLED/BUILD/). #if KIOCORE_BUILD_DEPRECATED_SINCE(5, 68) |
src/core/statjob.h | ||
79 ↗ | (On Diff #71701) | I guess this overload exists because of the setDetails(short int) overload? If so, I suppose we can get rid of it for Qt6. |
227 | This was marked as done, but I still see ", KIO::StatDetails int," | |
235 | Still there |
Add a test for setDetails(KIO::StatDetails), fix comment, add a KIOCORE_ENABLE_DEPRECATED_SINCE
Make setDetails(KIO::StatDetail detail) compile only when deprecated since 5.68 is enabled
Get code ready for 5.69, I really HOPE that the last time I have to do this, remove a few qDebug(), add some test assertions, make test more reliable
Huh. Sorry about the delays and the need to update version numbers once more.
My last two comments are still not fixed. To see the issues, grep for "direcly" ('t' missing) and "Details int" (comma missing? or "int" should be removed?).
Or just land as is and I'll fix them in a followup commit, it'll be faster...
Fix typo, make StatJob::setDetails(KIO::StatDetail detail) build only when build_deprecated(5.69) is set
This doesn't seem to compile:
[ 26%] Building CXX object src/ioslaves/file/CMakeFiles/kio_file.dir/file.cpp.o cd /usr/src/kio-git/src/build/src/ioslaves/file && /usr/lib/ccache/bin/c++ -DKCOMPLETION_DISABLE_DEPRECATED_BEFORE_AND_AT=0x054100 -DKCOREADDONS_LIB -DKF_DEPRECATED_WARNINGS_SINCE=0x060000 -DKF_DISABLE_DEPRECATED_BEFORE_AND_AT=0x054300 -DKIOCORE_DEPRECATED_WARNINGS_SINCE=0x0 -DKIOCORE_DISABLE_DEPRECATED_BEFORE_AND_AT=0x0 -DQT_CONCURRENT_LIB -DQT_CORE_LIB -DQT_DBUS_LIB -DQT_DEPRECATED_WARNINGS_SINCE=0x060000 -DQT_DISABLE_DEPRECATED_BEFORE=0x050d00 -DQT_NETWORK_LIB -DQT_NO_CAST_FROM_ASCII -DQT_NO_CAST_FROM_BYTEARRAY -DQT_NO_CAST_TO_ASCII -DQT_NO_DEBUG -DQT_NO_NARROWING_CONVERSIONS_IN_CONNECT -DQT_NO_SIGNALS_SLOTS_KEYWORDS -DQT_NO_URL_CAST_FROM_STRING -DQT_STRICT_ITERATORS -DQT_USE_QSTRINGBUILDER -DTRANSLATION_DOMAIN=\"kio5\" -D_GNU_SOURCE -D_LARGEFILE64_SOURCE -Dkio_file_EXPORTS -I/home/al/.cache/yay/kio-git/src/build/src/ioslaves/file -I/home/al/.cache/yay/kio-git/src/kio/src/ioslaves/file -I/home/al/.cache/yay/kio-git/src/build/src/ioslaves/file/kio_file_autogen/include -I/home/al/.cache/yay/kio-git/src/build/src/core/.. -I/home/al/.cache/yay/kio-git/src/kio/src/core/kssl -I/home/al/.cache/yay/kio-git/src/build/src/core -I/home/al/.cache/yay/kio-git/src/kio/src/core -isystem /usr/include/KF5/KCoreAddons -isystem /usr/include/KF5-isystem /usr/include/qt -isystem /usr/include/qt/QtCore -isystem /usr/lib/qt/mkspecs/linux-g++ -isystem /usr/include/KF5/KService -isystem /usr/include/KF5/KConfigCore -isystem /usr/include/qt/QtNetwork -isystem /usr/include/qt/QtConcurrent -isystem /usr/include/qt/QtDBus -isystem /usr/include/KF5/KI18n -isystem /usr/include/KF5/KAuth -march=x86-64 -mtune=native -O2 -pipe -fno-plt -g -fvar-tracking-assignments -fdebug-prefix-map=/home/al/.cache/yay/kio-git/src=/usr/src/debug -fno-operator-names -fno-exceptions -Wall -Wextra -Wcast-align -Wchar-subscripts -Wformat-security -Wno-long-long -Wpointer-arith -Wundef -Wnon-virtual-dtor -Woverloaded-virtual -Werror=return-type -Wvla -Wdate-time -Wsuggest-override -Wlogical-op -pedantic -Wzero-as-null-pointer-constant -O3 -DNDEBUG -fPIC -fvisibility=hidden -fvisibility-inlines-hidden -fPIC -std=gnu++11 -o CMakeFiles/kio_file.dir/file.cpp.o -c /home/al/.cache/yay/kio-git/src/kio/src/ioslaves/file/file.cpp /usr/src/kio-git/src/kio/src/ioslaves/file/file.cpp: In member function 'bool FileProtocol::createUDSEntry(const QString&, const QByteArray&, KIO::UDSEntry&, KIO::StatDetails)': /usr/src/kio-git/src/kio/src/ioslaves/file/file.cpp:1041:38: error: 'linkTargetBuffer' was not declared in this scope 1041 | targetPath = linkTargetBuffer; | ^~~~~~~~~~~~~~~~ make[2]: *** [src/ioslaves/file/CMakeFiles/kio_file.dir/build.make:76: src/ioslaves/file/CMakeFiles/kio_file.dir/file.cpp.o] Error 1
"/compile/kde5/framework/kde/kdegames/kjumpingcube/game.cpp:582:63: erreur: impossible de convertir « KIO::StatJob::DestinationSide » de « KIO::StatJob::StatSide » vers « KIO::JobFlags » {aka « QFlags<KIO::JobFlag> »}
582 | KIO::StatJob* statJob = KIO::stat(url, KIO::StatJob::DestinationSide, 0); | ~~~~~~~~~~~~~~^~~~~~~~~~~~~~~ | | | KIO::StatJob::StatSide
/compile/kde5/framework/kde/kdegames/kjumpingcube/game.cpp: Dans la fonction membre « void Game::loadGame() »:
/compile/kde5/framework/kde/kdegames/kjumpingcube/game.cpp:627:60: erreur: impossible de convertir « KIO::StatJob::SourceSide » de « KIO::StatJob::StatSide » vers « KIO::JobFlags » {aka « QFlags<KIO::JobFlag> »}
627 | KIO::StatJob* statJob = KIO::stat(url, KIO::StatJob::SourceSide, 0); | ~~~~~~~~~~~~~~^~~~~~~~~~ | | | KIO::StatJob::StatSide
"
compile is broken in several package see below.
@mlaurent compilation is broken because a method got deprecated, and you set -DKF_DISABLE_DEPRECATED_BEFORE_AND_AT=0x060000 so the use of that method no longer compiles.
You can hardly blame other people for deprecating methods, it's a rather natural thing to do at this point...
On the contrary, I strongly suggest to stop setting -DKF_DISABLE_DEPRECATED_BEFORE_AND_AT=0x060000, it just breaks too much. It even breaks stable branches.
I fixed that for kjumpingcube in https://commits.kde.org/kjumpingcube/5f238b424da1c57c56024a7e871579075bd06c73
In order to port away from deprecated methods, I suggest a script that increases the KF_DISABLE_DEPRECATED_BEFORE_AND_AT, which you (or anyone who wants to help with that) can run and fix compilation before pushing. It's what I regularly do for KF5 itself.
Did you test to compile with flags == 5.68.0 ?
I tested it with kdepim and I need to adapt it.
-DKF_DISABLE_DEPRECATED_BEFORE_AND_AT=0x054400 is 5.68.0 ?
src/core/statjob.h | ||
---|---|---|
198 ↗ | (On Diff #71701) | This wants to be "5, 69" now, not "5, 68" |
src/core/statjob.h | ||
---|---|---|
198 ↗ | (On Diff #71701) | Ah, was already fixed. |
You also want to adapt kdepim-runtime it seems -> failing as part of dependency build e.g. here: https://build.kde.org/view/Failing/job/Administration/job/Dependency%20Build%20Extragear%20kf5-qt5%20SUSEQt5.12/lastFailedBuild/console (I just triggered product build, but missed that the latest "Applications" dep build might not yet have pulled in the new KIO with the respective deprecation).
src/core/statjob.h | ||
---|---|---|
198 ↗ | (On Diff #71701) |
src/core/global.h | ||
---|---|---|
319 ↗ | (On Diff #71701) | This injects generic terms like Basic, User, Time, Acl, etc. into the KIO namespace, with no futher hint that these belong to this very enum, resulting in potential wrong usages (due to completion-based coding when being convertable to int) or in potential conflicts with other future additions. Sadly no time to follow the review. Had this been discussed before? Ideally those flags would get more explicit names, like BasicDetail (hm, what is basic actually), UserDetail, etc. Could not find naming recommendations for current Qt, but here some old one, scroll to the section "Naming Enum Types and Values": https://doc.qt.io/archives/qq/qq13-apis.html#theartofnaming |
src/core/global.h | ||
---|---|---|
319 ↗ | (On Diff #71701) | Yes it had been discussed and what you suggest was in a previous iteration : https://phabricator.kde.org/D25010?vs=69181&id=69385&whitespace=ignore-most#change-FL2qoDJhfEB5 Somehow I removed it : But I can't find why I did this, I guess it was a merging/synchronizing issue and lost those changes. Unless @dfaure you find a reason why I shouldn't do it, I will add those prefix back, since KF 5.69 is not yet out of the door, it is possible. |
I propose to go with prefixes again, for consistency with currrent enum usages in KF/KIO APIs.