From 268d2ffe8b533890b408740db1a6a3730dfd8597 Mon Sep 17 00:00:00 2001 From: Tannin Date: Sun, 17 May 2015 14:03:01 +0200 Subject: - some code cleanup and modernization trying to fix "dr memory" reports (though they were almost certainly false positives) - there is now a 50ms timeout on logging messages - bugfix: leaked handles after directory searches --- src/executableslist.cpp | 12 ++++---- src/logbuffer.cpp | 19 +++++++++---- src/modinfo.cpp | 9 +++--- src/organizercore.cpp | 2 +- src/shared/directoryentry.cpp | 64 +++++++++++++++++++++++-------------------- src/shared/directoryentry.h | 3 +- src/shared/util.cpp | 19 +++++++++++-- src/shared/util.h | 2 ++ 8 files changed, 81 insertions(+), 49 deletions(-) (limited to 'src') diff --git a/src/executableslist.cpp b/src/executableslist.cpp index 1852b0ad..badf0813 100644 --- a/src/executableslist.cpp +++ b/src/executableslist.cpp @@ -113,9 +113,9 @@ const Executable &ExecutablesList::find(const QString &title) const Executable &ExecutablesList::find(const QString &title) { - for (std::vector::iterator iter = m_Executables.begin(); iter != m_Executables.end(); ++iter) { - if (QString::compare(iter->m_Title, title, Qt::CaseInsensitive) == 0) { - return *iter; + for (Executable &exe : m_Executables) { + if (QString::compare(exe.m_Title, title, Qt::CaseInsensitive) == 0) { + return exe; } } throw std::runtime_error("invalid name"); @@ -124,9 +124,9 @@ Executable &ExecutablesList::find(const QString &title) Executable &ExecutablesList::findByBinary(const QFileInfo &info) { - for (std::vector::iterator iter = m_Executables.begin(); iter != m_Executables.end(); ++iter) { - if (info == iter->m_BinaryInfo) { - return *iter; + for (Executable &exe : m_Executables) { + if (info == exe.m_BinaryInfo) { + return exe; } } throw std::runtime_error("invalid info"); diff --git a/src/logbuffer.cpp b/src/logbuffer.cpp index b58ef1de..0ddd927b 100644 --- a/src/logbuffer.cpp +++ b/src/logbuffer.cpp @@ -18,6 +18,7 @@ along with Mod Organizer. If not, see . */ #include "logbuffer.h" +#include #include #include #include @@ -127,7 +128,15 @@ char LogBuffer::msgTypeID(QtMsgType type) void LogBuffer::log(QtMsgType type, const QMessageLogContext &context, const QString &message) { - QMutexLocker guard(&s_Mutex); + // QMutexLocker doesn't support timeout... + if (!s_Mutex.tryLock(50)) { + fprintf(stderr, "failed to log: %s", qPrintable(message)); + return; + } + ON_BLOCK_EXIT([] () { + s_Mutex.unlock(); + }); + if (!s_Instance.isNull()) { s_Instance->logMessage(type, message); } @@ -192,9 +201,9 @@ QVariant LogBuffer::data(const QModelIndex &index, int role) const switch (role) { case Qt::DisplayRole: { if (index.column() == 0) { - return m_Messages.at(msgIndex).time; + return m_Messages[msgIndex].time; } else if (index.column() == 1) { - const QString &msg = m_Messages.at(msgIndex).message; + const QString &msg = m_Messages[msgIndex].message; if (msg.length() < 200) { return msg; } else { @@ -204,7 +213,7 @@ QVariant LogBuffer::data(const QModelIndex &index, int role) const } break; case Qt::DecorationRole: { if (index.column() == 1) { - switch (m_Messages.at(msgIndex).type) { + switch (m_Messages[msgIndex].type) { case QtDebugMsg: return QIcon(":/MO/gui/information"); case QtWarningMsg: return QIcon(":/MO/gui/warning"); case QtCriticalMsg: return QIcon(":/MO/gui/important"); @@ -214,7 +223,7 @@ QVariant LogBuffer::data(const QModelIndex &index, int role) const } break; case Qt::UserRole: { if (index.column() == 1) { - switch (m_Messages.at(msgIndex).type) { + switch (m_Messages[msgIndex].type) { case QtDebugMsg: return "D"; case QtWarningMsg: return "W"; case QtCriticalMsg: return "C"; diff --git a/src/modinfo.cpp b/src/modinfo.cpp index 923ad855..bbc72c72 100644 --- a/src/modinfo.cpp +++ b/src/modinfo.cpp @@ -277,11 +277,10 @@ int ModInfo::checkAllForUpdate(QObject *receiver) modIDs.push_back(GameInfo::instance().getNexusModID()); - for (std::vector::iterator iter = s_Collection.begin(); - iter != s_Collection.end(); ++iter) { - if ((*iter)->canBeUpdated()) { - modIDs.push_back((*iter)->getNexusID()); - if (modIDs.size() >= 255) { + for (const ModInfo::Ptr &mod : s_Collection) { + if (mod->canBeUpdated()) { + modIDs.push_back(mod->getNexusID()); + if (modIDs.size() >= chunkSize) { checkChunkForUpdate(modIDs, receiver); modIDs.clear(); } diff --git a/src/organizercore.cpp b/src/organizercore.cpp index ce0b8c5f..c065e587 100644 --- a/src/organizercore.cpp +++ b/src/organizercore.cpp @@ -1182,7 +1182,7 @@ void OrganizerCore::updateModActiveState(int index, bool active) { ModInfo::Ptr modInfo = ModInfo::getByIndex(index); QDir dir(modInfo->absolutePath()); - foreach (const QString &esm, dir.entryList(QStringList() << "*.esm", QDir::Files)) { + for (const QString &esm : dir.entryList(QStringList() << "*.esm", QDir::Files)) { m_PluginList.enableESP(esm, active); } int enabled = 0; diff --git a/src/shared/directoryentry.cpp b/src/shared/directoryentry.cpp index 9a864245..248c4789 100644 --- a/src/shared/directoryentry.cpp +++ b/src/shared/directoryentry.cpp @@ -18,19 +18,20 @@ along with Mod Organizer. If not, see . */ #include "directoryentry.h" +#include "windows_error.h" +#include "leaktrace.h" +#include "error_report.h" +#include +#include +#include +#include #define WIN32_LEAN_AND_MEAN #include #include #include #include -#include -#include "windows_error.h" -#include -#include -#include -#include "leaktrace.h" #include -#include "error_report.h" +#include namespace MOShared { @@ -327,7 +328,7 @@ FileEntry::FileEntry() } FileEntry::FileEntry(Index index, const std::wstring &name, DirectoryEntry *parent) - : m_Index(index), m_Name(name), m_Origin(-1), m_Parent(parent), m_Archive(L""), m_LastAccessed(time(nullptr)) + : m_Index(index), m_Name(name), m_Origin(-1), m_Archive(L""), m_Parent(parent), m_LastAccessed(time(nullptr)) { LEAK_TRACE; } @@ -414,8 +415,8 @@ const std::wstring &DirectoryEntry::getName() const void DirectoryEntry::clear() { m_Files.clear(); - for (std::vector::iterator iter = m_SubDirectories.begin(); iter != m_SubDirectories.end(); ++iter) { - delete *iter; + for (DirectoryEntry *entry : m_SubDirectories) { + delete entry; } m_SubDirectories.clear(); } @@ -583,11 +584,11 @@ void DirectoryEntry::removeDirRecursive() m_FileRegister->removeFile(m_Files.begin()->second); } - for (auto iter = m_SubDirectories.begin(); iter != m_SubDirectories.end(); ++iter) { - (*iter)->removeDirRecursive(); - delete *iter; + for (DirectoryEntry *entry : m_SubDirectories) { + entry->removeDirRecursive(); + delete entry; } - m_SubDirectories.clear(); + m_SubDirectories.clear(); } void DirectoryEntry::removeDir(const std::wstring &path) @@ -595,8 +596,8 @@ void DirectoryEntry::removeDir(const std::wstring &path) size_t pos = path.find_first_of(L"\\/"); if (pos == std::string::npos) { for (auto iter = m_SubDirectories.begin(); iter != m_SubDirectories.end(); ++iter) { - if (_wcsicmp((*iter)->getName().c_str(), path.c_str()) == 0) { - DirectoryEntry *entry = *iter; + DirectoryEntry *entry = *iter; + if (CaseInsensitiveEqual(entry->getName(), path)) { entry->removeDirRecursive(); m_SubDirectories.erase(iter); delete entry; @@ -622,7 +623,7 @@ void DirectoryEntry::insertFile(const std::wstring &filePath, FilesOrigin &origi { size_t pos = filePath.find_first_of(L"\\/"); if (pos == std::string::npos) { - this->insert(filePath, origin, fileTime, L""); + this->insert(filePath, origin, fileTime, std::wstring()); } else { std::wstring dirName = filePath.substr(0, pos); std::wstring rest = filePath.substr(pos + 1); @@ -669,15 +670,15 @@ int DirectoryEntry::anyOrigin() const bool ignore; for (auto iter = m_Files.begin(); iter != m_Files.end(); ++iter) { FileEntry::Ptr entry = m_FileRegister->getFile(iter->second); - if (!entry->isFromArchive()) { + if ((entry.get() != nullptr) && !entry->isFromArchive()) { return entry->getOrigin(ignore); } } // if we got here, no file directly within this directory is a valid indicator for a mod, thus // we continue looking in subdirectories - for (std::vector::const_iterator iter = m_SubDirectories.begin(); iter != m_SubDirectories.end(); ++iter) { - int res = (*iter)->anyOrigin(); + for (DirectoryEntry *entry : m_SubDirectories) { + int res = entry->anyOrigin(); if (res != -1){ return res; } @@ -761,6 +762,10 @@ const FileEntry::Ptr DirectoryEntry::searchFile(const std::wstring &path, const std::wstring pathComponent = path.substr(0, len); DirectoryEntry *temp = findSubDirectory(pathComponent); if (temp != nullptr) { + if (len >= path.size()) { + log("unexpected end of path"); + return FileEntry::Ptr(); + } return temp->searchFile(path.substr(len + 1), directory); } } @@ -770,9 +775,9 @@ const FileEntry::Ptr DirectoryEntry::searchFile(const std::wstring &path, const DirectoryEntry *DirectoryEntry::findSubDirectory(const std::wstring &name) const { - for (std::vector::const_iterator iter = m_SubDirectories.begin(); iter != m_SubDirectories.end(); ++iter) { - if (_wcsicmp((*iter)->getName().c_str(), name.c_str()) == 0) { - return *iter; + for (DirectoryEntry *entry : m_SubDirectories) { + if (CaseInsensitiveEqual(entry->getName(), name)) { + return entry; } } return nullptr; @@ -797,14 +802,14 @@ const FileEntry::Ptr DirectoryEntry::findFile(const std::wstring &name) const DirectoryEntry *DirectoryEntry::getSubDirectory(const std::wstring &name, bool create, int originID) { - for (std::vector::iterator iter = m_SubDirectories.begin(); iter != m_SubDirectories.end(); ++iter) { - if (_wcsicmp((*iter)->getName().c_str(), name.c_str()) == 0) { - return *iter; + for (DirectoryEntry *entry : m_SubDirectories) { + if (CaseInsensitiveEqual(entry->getName(), name)) { + return entry; } } if (create) { std::vector::iterator iter = m_SubDirectories.insert(m_SubDirectories.end(), - new DirectoryEntry(name, this, originID, m_FileRegister, m_OriginConnection)); + new DirectoryEntry(name, this, originID, m_FileRegister, m_OriginConnection)); return *iter; } else { return nullptr; @@ -849,7 +854,7 @@ FileRegister::~FileRegister() FileEntry::Index FileRegister::generateIndex() { - static FileEntry::Index sIndex = 0; + static std::atomic sIndex(0); return sIndex++; } @@ -871,8 +876,9 @@ FileEntry::Ptr FileRegister::getFile(FileEntry::Index index) const auto iter = m_Files.find(index); if (iter != m_Files.end()) { return iter->second; + } else { + return FileEntry::Ptr(); } - return FileEntry::Ptr(); } void FileRegister::unregisterFile(FileEntry::Ptr file) diff --git a/src/shared/directoryentry.h b/src/shared/directoryentry.h index 0075d5e5..2d32450a 100644 --- a/src/shared/directoryentry.h +++ b/src/shared/directoryentry.h @@ -236,7 +236,8 @@ public: std::vector getFiles() const; - void getSubDirectories(std::vector::const_iterator &begin, std::vector::const_iterator &end) const { + void getSubDirectories(std::vector::const_iterator &begin + , std::vector::const_iterator &end) const { begin = m_SubDirectories.begin(); end = m_SubDirectories.end(); } diff --git a/src/shared/util.cpp b/src/shared/util.cpp index e5bcd436..3692aae1 100644 --- a/src/shared/util.cpp +++ b/src/shared/util.cpp @@ -109,7 +109,7 @@ std::string &ToLower(std::string &text) std::string ToLower(const std::string &text) { - std::string result = text; + std::string result(text); std::transform(result.begin(), result.end(), result.begin(), locToLower); return result; } @@ -122,11 +122,26 @@ std::wstring &ToLower(std::wstring &text) std::wstring ToLower(const std::wstring &text) { - std::wstring result = text; + std::wstring result(text); std::transform(result.begin(), result.end(), result.begin(), locToLowerW); return result; } +bool CaseInsenstiveComparePred(wchar_t lhs, wchar_t rhs) +{ + return std::tolower(lhs, loc) == std::tolower(rhs, loc); +} + +bool CaseInsensitiveEqual(const std::wstring &lhs, const std::wstring &rhs) +{ + return (lhs.length() == rhs.length()) + && std::equal(lhs.begin(), lhs.end(), + rhs.begin(), + [] (wchar_t lhs, wchar_t rhs) -> bool { + return std::tolower(lhs, loc) == std::tolower(rhs, loc); + }); +} + VS_FIXEDFILEINFO GetFileVersion(const std::wstring &fileName) { DWORD handle = 0UL; diff --git a/src/shared/util.h b/src/shared/util.h index 14017526..1e498059 100644 --- a/src/shared/util.h +++ b/src/shared/util.h @@ -42,6 +42,8 @@ std::string ToLower(const std::string &text); std::wstring &ToLower(std::wstring &text); std::wstring ToLower(const std::wstring &text); +bool CaseInsensitiveEqual(const std::wstring &lhs, const std::wstring &rhs); + VS_FIXEDFILEINFO GetFileVersion(const std::wstring &fileName); } // namespace MOShared -- cgit v1.3.1