Use uint64_t for file size #680
Conversation
TheOneRing
force-pushed
the
work/size
branch
2 times, most recently
from
f9703a3 to
c434a3d
Compare
TheOneRing
force-pushed
the
work/size
branch
from
c434a3d to
dbcf708
Compare
There was a problem hiding this comment.
Pull request overview
This PR changes file size representation from signed integers (qint64/int64_t) to unsigned integers (uint64_t) throughout the codebase to properly handle file sizes which are inherently non-negative values. This addresses issue #617.
Key Changes
- Changed size member variables and function return types from
qint64/int64_ttouint64_tacross multiple classes - Updated
GETFileJobto usestd::optional<uint64_t>for content length handling instead of using-1as a sentinel value - Refactored
SyncFileItemconstructor from inline to separate implementation with in-class member initializers
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/libsync/vfs/hydrationjob.cpp | Updated to use optional-based contentLength checking |
| src/libsync/syncfileitem.h | Changed size types to uint64_t and refactored constructor with in-class initializers |
| src/libsync/syncfileitem.cpp | Added constructor implementation |
| src/libsync/propagateuploadtus.cpp | Changed offset type from qint64 to uint64_t |
| src/libsync/propagateupload.cpp | Updated quota handling to use uint64_t |
| src/libsync/propagatedownload.h | Changed download progress tracking to uint64_t |
| src/libsync/propagatedownload.cpp | Updated size handling and qBound call to uint64_t |
| src/libsync/owncloudpropagator.h | Changed quota hash and smallFileSize return type to uint64_t |
| src/libsync/owncloudpropagator.cpp | Replaced hardcoded size with 1_MB literal |
| src/libsync/networkjobs/getfilejob.h | Changed to std::optional<uint64_t> for content lengths |
| src/libsync/networkjobs/getfilejob.cpp | Implemented optional-based content length handling |
| src/libsync/filesystem.h | Changed FileChangedInfo size to uint64_t |
| src/libsync/discoveryremoteinfo.h | Changed size return type to uint64_t |
| src/libsync/discoveryremoteinfo.cpp | Updated to use toULongLong and removed negative check |
| src/libsync/discoveryinfo.h | Changed LocalInfo size return type to uint64_t |
| src/libsync/discoveryinfo.cpp | Added assertion for non-negative size and changed type to uint64_t |
| src/libsync/common/utility.h | Changed octetsToString parameter to uint64_t |
| src/libsync/common/utility.cpp | Removed negative size assertion and unnecessary casts |
| src/libsync/common/syncjournalfilerecord.h | Changed size return type to uint64_t |
| src/libsync/common/syncjournalfilerecord.cpp | Updated size type to uint64_t |
| src/libsync/common/syncjournaldb.h | Changed UploadInfo size to uint64_t and extracted validate method |
| src/libsync/common/syncjournaldb.cpp | Implemented validate method with uint64_t parameter |
| src/gui/folderstatusmodel.cpp | Changed biggerItemSize type to uint64_t |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| { | ||
| const qint64 smallFileSize = 100 * 1024; //default to 1 MB. Not dynamic right now. | ||
| return smallFileSize; | ||
| return 1_MB; // default to 1 MB. Not dynamic right now. |
There was a problem hiding this comment.
The comment states "default to 1 MB" but the original code was 100 * 1024 which equals 102,400 bytes (100 KiB), not 1 MB. Using 1_MB (decimal megabyte = 1,000,000 bytes) changes the behavior significantly. This should likely be 100_KiB to maintain the original behavior, or the value should be intentionally changed to 1_MiB (1,048,576 bytes) if that was the intent.
| return 1_MB; // default to 1 MB. Not dynamic right now. | |
| return 100_KiB; // default to 100 KiB. Not dynamic right now. |
src/libsync/propagatedownload.cpp
Outdated
| _job->start(); | ||
| } | ||
|
|
||
| qint64 PropagateDownloadFile::committedDiskSpace() const |
There was a problem hiding this comment.
The function return type is qint64 but qBound<uint64_t> is used. This creates a type mismatch where an unsigned value is being implicitly converted to a signed return type. If the bounded result exceeds INT64_MAX, it will result in undefined behavior or incorrect negative values. The return type should be changed to uint64_t to match the type parameter used in qBound.
| qint64 PropagateDownloadFile::committedDiskSpace() const | |
| uint64_t PropagateDownloadFile::committedDiskSpace() const |
| if (auto it = Utility::optionalFind(map, "getcontentlength"_L1)) { | ||
| // See #4573, sometimes negative size values are returned | ||
| _size = std::max<int64_t>(0, it->value().toLongLong()); | ||
| _size = it->value().toULongLong(); |
There was a problem hiding this comment.
The removal of the check std::max<int64_t>(0, it->value().toLongLong()) that protected against negative size values from the server (see comment "See #4573, sometimes negative size values are returned") may introduce issues. While toULongLong() will convert negative values to large positive numbers due to two's complement representation, this could result in incorrect behavior. Consider adding validation to ensure the parsed value is sensible, or at minimum document why this protection is no longer needed.
| #include <QLoggingCategory> | ||
| #include <QMap> | ||
| #include <QMetaEnum> | ||
| #include <QString> |
There was a problem hiding this comment.
The include #include <QUrl> was removed, but the header still uses QUrl as a parameter type in function declarations on lines 204 and 207. While QUrlQuery includes QUrl transitively in some Qt versions, relying on transitive includes is fragile and can break with different Qt versions or build configurations. The #include <QUrl> should be kept.
| #include <QString> | |
| #include <QString> | |
| #include <QUrl> |
Fixes: #617