From c4e880622b02f1d993b55f04f0559c158dc3a51f Mon Sep 17 00:00:00 2001 From: Magnus Gille Date: Sat, 27 Dec 2025 11:06:45 -0800 Subject: [PATCH] QZip improvements (#4768) Removed unused constructors Replaced raw pointer with a unique_ptr to clearly pass ownership. Fixed a bug where we compared if a signed integer is negative (it never is). Fixed a case where we take the pointer to a pointer we de-reference! --- contrib/qzip/zip.cpp | 62 +++++++++++++++++++--------------------- contrib/qzip/zipreader.h | 5 ++-- contrib/qzip/zipwriter.h | 7 ++--- src/Gui/IconManager.cpp | 16 +++++------ src/Gui/IconManager.h | 4 ++- 5 files changed, 45 insertions(+), 49 deletions(-) diff --git a/contrib/qzip/zip.cpp b/contrib/qzip/zip.cpp index 5326c7687..f141462b7 100644 --- a/contrib/qzip/zip.cpp +++ b/contrib/qzip/zip.cpp @@ -397,21 +397,22 @@ bool ZipReader::FileInfo::isValid() const class QZipPrivate { public: - QZipPrivate(QIODevice *device, bool ownDev) - : device(device), ownDevice(ownDev), dirtyFileTree(true), start_of_directory(0) + QZipPrivate(QIODevice *device) + : device(device), dirtyFileTree(true), start_of_directory(0) { } - ~QZipPrivate() + QZipPrivate(std::unique_ptr dev) + : device(dev.get()), ownedDevice(std::move(dev)), dirtyFileTree(true), start_of_directory(0) { - if (ownDevice) - delete device; } + virtual ~QZipPrivate() = default; + void fillFileInfo(int index, ZipReader::FileInfo &fileInfo) const; QIODevice *device; - bool ownDevice; + std::unique_ptr ownedDevice; bool dirtyFileTree; QList fileHeaders; QByteArray comment; @@ -439,8 +440,13 @@ void QZipPrivate::fillFileInfo(int index, ZipReader::FileInfo &fileInfo) const class ZipReaderPrivate : public QZipPrivate { public: - ZipReaderPrivate(QIODevice *device, bool ownDev) - : QZipPrivate(device, ownDev), status(ZipReader::NoError) + ZipReaderPrivate(QIODevice *device) + : QZipPrivate(device), status(ZipReader::NoError) + { + } + + ZipReaderPrivate(std::unique_ptr device) + : QZipPrivate(std::move(device)), status(ZipReader::NoError) { } @@ -452,8 +458,16 @@ public: class ZipWriterPrivate : public QZipPrivate { public: - ZipWriterPrivate(QIODevice *device, bool ownDev) - : QZipPrivate(device, ownDev), + ZipWriterPrivate(QIODevice *device) + : QZipPrivate(device), + status(ZipWriter::NoError), + permissions(QFile::ReadOwner | QFile::WriteOwner), + compressionPolicy(ZipWriter::AlwaysCompress) + { + } + + ZipWriterPrivate(std::unique_ptr device) + : QZipPrivate(std::move(device)), status(ZipWriter::NoError), permissions(QFile::ReadOwner | QFile::WriteOwner), compressionPolicy(ZipWriter::AlwaysCompress) @@ -755,19 +769,14 @@ ZipReader::ZipReader(const QString &archive, QIODevice::OpenMode mode) status = FileError; } - d = new ZipReaderPrivate(&(*f.release()), /*ownDevice=*/true); + d = std::make_unique(std::move(f)); d->status = status; } -/*! - Create a new zip archive that operates on the archive found in \a device. - You have to open the device previous to calling the constructor and only a - device that is readable will be scanned for zip filecontent. - */ -ZipReader::ZipReader(QIODevice *device) - : d(new ZipReaderPrivate(device, /*ownDevice=*/false)) +ZipReader::ZipReader(std::unique_ptr device) + : d(std::make_unique(std::move(device))) { - Q_ASSERT(device); + Q_ASSERT(d->device); } /*! @@ -776,7 +785,6 @@ ZipReader::ZipReader(QIODevice *device) ZipReader::~ZipReader() { close(); - delete d; } /*! @@ -1039,25 +1047,13 @@ ZipWriter::ZipWriter(const QString &fileName, QIODevice::OpenMode mode) status = ZipWriter::FileError; } - d = new ZipWriterPrivate(&(*f.release()), /*ownDevice=*/true); + d = std::make_unique(std::move(f)); d->status = status; } -/*! - Create a new zip archive that operates on the archive found in \a device. - You have to open the device previous to calling the constructor and - only a device that is readable will be scanned for zip filecontent. - */ -ZipWriter::ZipWriter(QIODevice *device) - : d(new ZipWriterPrivate(device, /*ownDevice=*/false)) -{ - Q_ASSERT(device); -} - ZipWriter::~ZipWriter() { close(); - delete d; } /*! diff --git a/contrib/qzip/zipreader.h b/contrib/qzip/zipreader.h index 0bcca2abd..b160095a3 100644 --- a/contrib/qzip/zipreader.h +++ b/contrib/qzip/zipreader.h @@ -67,8 +67,8 @@ class ZipReader { public: ZipReader(const QString &fileName, QIODevice::OpenMode mode = QIODevice::ReadOnly ); + explicit ZipReader(std::unique_ptr device); - explicit ZipReader(QIODevice *device); ~ZipReader(); QIODevice* device() const; @@ -114,8 +114,7 @@ public: void close(); private: - ZipReaderPrivate *d; - Q_DISABLE_COPY(ZipReader) + std::unique_ptr d; }; QT_END_NAMESPACE diff --git a/contrib/qzip/zipwriter.h b/contrib/qzip/zipwriter.h index 9d7d1d7d5..a6b4225e7 100644 --- a/contrib/qzip/zipwriter.h +++ b/contrib/qzip/zipwriter.h @@ -66,7 +66,6 @@ class ZipWriter public: ZipWriter(const QString &fileName, QIODevice::OpenMode mode = (QIODevice::WriteOnly | QIODevice::Truncate) ); - explicit ZipWriter(QIODevice *device); ~ZipWriter(); QIODevice* device() const; @@ -105,9 +104,9 @@ public: void addSymLink(const QString &fileName, const QString &destination); void close(); -private: - ZipWriterPrivate *d; - Q_DISABLE_COPY(ZipWriter) + + private: + std::unique_ptr d; }; QT_END_NAMESPACE diff --git a/src/Gui/IconManager.cpp b/src/Gui/IconManager.cpp index 48fe32958..34f451502 100644 --- a/src/Gui/IconManager.cpp +++ b/src/Gui/IconManager.cpp @@ -208,8 +208,8 @@ bool IconManager::importBundle (const QString &filename) { - QFile zipFile(filename); - return importBundle(&zipFile); + auto qfile = std::make_unique(filename); + return importBundle(std::move(qfile)); } @@ -218,19 +218,19 @@ IconManager::importBundle (const QUrl &url) { QByteArray zipData = downloadUrl(url); - QBuffer buffer; - buffer.setData(zipData); - buffer.open(QIODevice::ReadOnly); + auto buffer = std::make_unique(); + buffer->setData(zipData); + buffer->open(QIODevice::ReadOnly); - return importBundle(&buffer); + return importBundle(std::move(buffer)); } bool IconManager::importBundle -(QIODevice *device) +(std::unique_ptr device) { - ZipReader reader(device); + ZipReader reader(std::move(device)); for (ZipReader::FileInfo info : reader.fileInfoList()) { if (info.isFile) { QByteArray data = reader.fileData(info.filePath); diff --git a/src/Gui/IconManager.h b/src/Gui/IconManager.h index 520b083c5..bb57411e3 100644 --- a/src/Gui/IconManager.h +++ b/src/Gui/IconManager.h @@ -19,6 +19,8 @@ #ifndef _GC_IconManager_h #define _GC_IconManager_h +#include + #include #include #include @@ -52,7 +54,7 @@ public: bool exportBundle(const QString &filepath); bool importBundle(const QString &filepath); bool importBundle(const QUrl &url); - bool importBundle(QIODevice *device); + bool importBundle(std::unique_ptr device); bool saveConfig() const;