feverfew requested changes to this revision. feverfew added a comment. This revision now requires changes to proceed.
Good stuff, I'll admit I kind of skimmed over the bits that were rewrites `error(...); return;` -> `Result::fail()`, I assume you've mapped correctly there. Only minor comments from me. INLINE COMMENTS > kio_sftp.cpp:145 > KIO::fileoffset_t offset = -1; > while ((offset = QT_LSEEK(fd, pos, mode)) == EAGAIN); > return offset; Technically unrelated to your changes, but does this even work? isn't the check supposed to be against `errno` in this case, not offset? > kio_sftp.cpp:302 > +{ > + if (!result.success) { > + error(result.error, result.errorString); `if (result.success) { ...} else { ... } seems clearer to me > kio_sftp.cpp:328 > + for (int iInt = 0; iInt < n; ++iInt) { > + unsigned int i = static_cast<unsigned int>(iInt); // can only be > >0 > char echo; I'm a bit confused, why is this necessary? > kio_sftp.cpp:418 > long long fileType = QT_STAT_REG; > - long long size = 0LL; > + uint64_t size = 0LL; > Should then be changed from `0LL` to `0U`? > kio_sftp.cpp:1493 > > -void sftpProtocol::close() { > - closeWithoutFinish(); > - finished(); > +void SFTPInternal::close() > +{ We should be calling `finished()` (or `Result::pass`) on `close()` IIRC? I'm not seeing it called. > kio_sftp.h:77 > + void setHost(const QString &h, quint16 port, const QString& user, const > QString& pass); > + Q_REQUIRED_RESULT Result get(const QUrl &url); > + Q_REQUIRED_RESULT Result listDir(const QUrl &url); TODO KF6 https://en.cppreference.com/w/cpp/language/attributes/nodiscard ? REPOSITORY R320 KIO Extras REVISION DETAIL https://phabricator.kde.org/D27153 To: sitter, dfaure, feverfew Cc: kde-frameworks-devel, kfm-devel, pberestov, iasensio, fprice, LeGast00n, cblack, MrPepe, fbampaloukas, alexde, GB_2, Codezela, feverfew, meven, michaelh, spoorun, navarromorales, firef, ngraham, andrebarros, bruns, emmanuelp, mikesomov