From 5e98cb139a81ea9a124b1100fa710514a5f95c00 Mon Sep 17 00:00:00 2001 From: Lioncash Date: Sat, 24 Aug 2019 15:23:54 -0400 Subject: [PATCH 1/8] hecl/hecl: Convert std::string/std::wstring to views Gets rid of two static constructors. --- hecl/lib/hecl.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/hecl/lib/hecl.cpp b/hecl/lib/hecl.cpp index f16a41bda..445eb351b 100644 --- a/hecl/lib/hecl.cpp +++ b/hecl/lib/hecl.cpp @@ -24,7 +24,7 @@ namespace hecl { unsigned VerbosityLevel = 0; bool GuiMode = false; logvisor::Module LogModule("hecl"); -static const std::string Illegals{"<>?\""}; +constexpr std::string_view Illegals{"<>?\""}; void SanitizePath(std::string& path) { if (path.empty()) @@ -54,7 +54,7 @@ void SanitizePath(std::string& path) { path.pop_back(); } -static const std::wstring WIllegals{L"<>?\""}; +constexpr std::wstring_view WIllegals{L"<>?\""}; void SanitizePath(std::wstring& path) { if (path.empty()) From e96eb4cac6935f6b468e85eb3beec232f75dd472 Mon Sep 17 00:00:00 2001 From: Lioncash Date: Sat, 24 Aug 2019 15:27:05 -0400 Subject: [PATCH 2/8] hecl/hecl: Include relevant headers Ensures the translation unit includes exactly what it needs. --- hecl/lib/hecl.cpp | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/hecl/lib/hecl.cpp b/hecl/lib/hecl.cpp index 445eb351b..c64a4ca24 100644 --- a/hecl/lib/hecl.cpp +++ b/hecl/lib/hecl.cpp @@ -1,6 +1,14 @@ #include "hecl/hecl.hpp" -#include + +#include +#include +#include +#include +#include #include +#include +#include +#include #include #ifdef WIN32 @@ -20,6 +28,8 @@ #include #endif +#include + namespace hecl { unsigned VerbosityLevel = 0; bool GuiMode = false; From 32fec587b5f3a57eb2247cd98929b663017d6e00 Mon Sep 17 00:00:00 2001 From: Lioncash Date: Sat, 24 Aug 2019 15:32:15 -0400 Subject: [PATCH 3/8] hecl/hecl: Collapse InProgress() into std::any_of() Same thing, more straightforward. --- hecl/lib/hecl.cpp | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/hecl/lib/hecl.cpp b/hecl/lib/hecl.cpp index c64a4ca24..85550b49f 100644 --- a/hecl/lib/hecl.cpp +++ b/hecl/lib/hecl.cpp @@ -129,10 +129,8 @@ static std::unordered_map PathsInProgress; bool ResourceLock::InProgress(const ProjectPath& path) { std::unique_lock lk{PathsMutex}; - for (const auto& p : PathsInProgress) - if (p.second == path) - return true; - return false; + return std::any_of(PathsInProgress.cbegin(), PathsInProgress.cend(), + [&path](const auto& entry) { return entry.second == path; }); } bool ResourceLock::SetThreadRes(const ProjectPath& path) { From bc7a6563cf947e03e1750ba135385832b3803043 Mon Sep 17 00:00:00 2001 From: Lioncash Date: Sat, 24 Aug 2019 15:36:35 -0400 Subject: [PATCH 4/8] hecl/hecl: Simplify SetThreadRes() --- hecl/lib/hecl.cpp | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/hecl/lib/hecl.cpp b/hecl/lib/hecl.cpp index 85550b49f..1151e2833 100644 --- a/hecl/lib/hecl.cpp +++ b/hecl/lib/hecl.cpp @@ -135,14 +135,17 @@ bool ResourceLock::InProgress(const ProjectPath& path) { bool ResourceLock::SetThreadRes(const ProjectPath& path) { std::unique_lock lk{PathsMutex}; - if (PathsInProgress.find(std::this_thread::get_id()) != PathsInProgress.cend()) + if (PathsInProgress.find(std::this_thread::get_id()) != PathsInProgress.cend()) { LogModule.report(logvisor::Fatal, fmt("multiple resource locks on thread")); + } - for (const auto& p : PathsInProgress) - if (p.second == path) - return false; + const bool isInProgress = std::any_of(PathsInProgress.cbegin(), PathsInProgress.cend(), + [&path](const auto& entry) { return entry.second == path; }); + if (isInProgress) { + return false; + } - PathsInProgress[std::this_thread::get_id()] = path; + PathsInProgress.insert_or_assign(std::this_thread::get_id(), path); return true; } From 0a6edbad2c0b9d45926a864df355ff90e41366e2 Mon Sep 17 00:00:00 2001 From: Lioncash Date: Sat, 24 Aug 2019 15:47:46 -0400 Subject: [PATCH 5/8] hecl/hecl: Tidy up GetSystemLocations() Converts a define into a constexpr variable, also joins declarations with assignments where applicable. --- hecl/lib/hecl.cpp | 56 ++++++++++++++++++++++------------------------- 1 file changed, 26 insertions(+), 30 deletions(-) diff --git a/hecl/lib/hecl.cpp b/hecl/lib/hecl.cpp index 1151e2833..c640643ef 100644 --- a/hecl/lib/hecl.cpp +++ b/hecl/lib/hecl.cpp @@ -442,8 +442,6 @@ hecl::DirectoryEnumerator::DirectoryEnumerator(SystemStringView path, Mode mode, #endif } -#define FILE_MAXDIR 768 - static std::pair NameFromPath(hecl::SystemStringView path) { hecl::SystemUTF8Conv utf8(path); if (utf8.str().size() == 1 && utf8.str()[0] == '/') @@ -461,46 +459,44 @@ std::vector> GetSystemLocations() { #if !WINDOWS_STORE /* Add the drive names to the listing (as queried by blender) */ { + constexpr uint32_t FILE_MAXDIR = 768; wchar_t wline[FILE_MAXDIR]; - wchar_t* name; - __int64 tmp; - int i; + const uint32_t tmp = GetLogicalDrives(); - tmp = GetLogicalDrives(); - - for (i = 0; i < 26; i++) { + for (uint32_t i = 0; i < 26; i++) { if ((tmp >> i) & 1) { wline[0] = L'A' + i; wline[1] = L':'; wline[2] = L'/'; wline[3] = L'\0'; - name = nullptr; + wchar_t* name = nullptr; /* Flee from horrible win querying hover floppy drives! */ if (i > 1) { /* Try to get volume label as well... */ if (GetVolumeInformationW(wline, wline + 4, FILE_MAXDIR - 4, nullptr, nullptr, nullptr, nullptr, 0)) { - size_t labelLen = wcslen(wline + 4); + const size_t labelLen = std::wcslen(wline + 4); _snwprintf(wline + 4 + labelLen, FILE_MAXDIR - 4 - labelLen, L" (%.2s)", wline); name = wline + 4; } } wline[2] = L'\0'; - if (name) - ret.emplace_back(wline, hecl::WideToUTF8(name)); - else + if (name == nullptr) { ret.push_back(NameFromPath(wline)); + } else { + ret.emplace_back(wline, hecl::WideToUTF8(name)); + } } } /* Adding Desktop and My Documents */ SystemString wpath; - SHGetSpecialFolderPathW(0, wline, CSIDL_PERSONAL, 0); + SHGetSpecialFolderPathW(nullptr, wline, CSIDL_PERSONAL, 0); wpath.assign(wline); SanitizePath(wpath); ret.push_back(NameFromPath(wpath)); - SHGetSpecialFolderPathW(0, wline, CSIDL_DESKTOPDIRECTORY, 0); + SHGetSpecialFolderPathW(nullptr, wline, CSIDL_DESKTOPDIRECTORY, 0); wpath.assign(wline); SanitizePath(wpath); ret.push_back(NameFromPath(wpath)); @@ -524,18 +520,20 @@ std::vector> GetSystemLocations() { /*https://developer.apple.com/library/mac/#documentation/CoreFOundation/Reference/CFURLRef/Reference/reference.html*/ /* we get all volumes sorted including network and do not relay on user-defined finder visibility, less confusing */ - CFURLRef cfURL = NULL; + CFURLRef cfURL = nullptr; CFURLEnumeratorResult result = kCFURLEnumeratorSuccess; - CFURLEnumeratorRef volEnum = CFURLEnumeratorCreateForMountedVolumes(NULL, kCFURLEnumeratorSkipInvisibles, NULL); + CFURLEnumeratorRef volEnum = + CFURLEnumeratorCreateForMountedVolumes(nullptr, kCFURLEnumeratorSkipInvisibles, nullptr); while (result != kCFURLEnumeratorEnd) { char defPath[1024]; - result = CFURLEnumeratorGetNextURL(volEnum, &cfURL, NULL); - if (result != kCFURLEnumeratorSuccess) + result = CFURLEnumeratorGetNextURL(volEnum, &cfURL, nullptr); + if (result != kCFURLEnumeratorSuccess) { continue; + } - CFURLGetFileSystemRepresentation(cfURL, false, (UInt8*)defPath, 1024); + CFURLGetFileSystemRepresentation(cfURL, false, reinterpret_cast(defPath), std::size(defPath)); ret.push_back(NameFromPath(defPath)); } @@ -593,13 +591,10 @@ std::wstring Char16ToWide(std::u16string_view src) { return std::wstring(src.beg #if _WIN32 int RecursiveMakeDir(const SystemChar* dir) { SystemChar tmp[1024]; - SystemChar* p = nullptr; - Sstat sb; - size_t len; /* copy path */ - wcsncpy(tmp, dir, 1024); - len = wcslen(tmp); + std::wcsncpy(tmp, dir, 1024); + const size_t len = std::wcslen(tmp); if (len >= 1024) { return -1; } @@ -610,6 +605,8 @@ int RecursiveMakeDir(const SystemChar* dir) { } /* recursive mkdir */ + SystemChar* p = nullptr; + Sstat sb; for (p = tmp + 1; *p; p++) { if (*p == '/' || *p == '\\') { *p = 0; @@ -641,13 +638,10 @@ int RecursiveMakeDir(const SystemChar* dir) { #else int RecursiveMakeDir(const SystemChar* dir) { SystemChar tmp[1024]; - SystemChar* p = nullptr; - Sstat sb; - size_t len; /* copy path */ - strncpy(tmp, dir, 1024); - len = strlen(tmp); + std::strncpy(tmp, dir, 1024); + const size_t len = std::strlen(tmp); if (len >= 1024) { return -1; } @@ -658,6 +652,8 @@ int RecursiveMakeDir(const SystemChar* dir) { } /* recursive mkdir */ + SystemChar* p = nullptr; + Sstat sb; for (p = tmp + 1; *p; p++) { if (*p == '/') { *p = 0; From 40b2e3edde49df9bca6981f7987a2571d7185011 Mon Sep 17 00:00:00 2001 From: Lioncash Date: Sat, 24 Aug 2019 15:54:37 -0400 Subject: [PATCH 6/8] hecl/hecl: Dehardcode sizes where applicable Queries the source arrays for the size instead of replicating it elsewhere. --- hecl/lib/hecl.cpp | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/hecl/lib/hecl.cpp b/hecl/lib/hecl.cpp index c640643ef..5808c38bf 100644 --- a/hecl/lib/hecl.cpp +++ b/hecl/lib/hecl.cpp @@ -100,8 +100,9 @@ SystemString GetcwdStr() { // const int MaxChunks=10240; // 2550 KiBs of current path are more than enough SystemChar stackBuffer[255]; // Stack buffer for the "normal" case - if (Getcwd(stackBuffer, 255) != nullptr) + if (Getcwd(stackBuffer, int(std::size(stackBuffer))) != nullptr) { return SystemString(stackBuffer); + } if (errno != ERANGE) { // It's not ERANGE, so we don't know how to handle it LogModule.report(logvisor::Fatal, fmt("Cannot determine the current path.")); @@ -111,9 +112,11 @@ SystemString GetcwdStr() { for (int chunks = 2; chunks < 10240; chunks++) { // With boost use scoped_ptr; in C++0x, use unique_ptr // If you want to be less C++ but more efficient you may want to use realloc - std::unique_ptr cwd(new SystemChar[255 * chunks]); - if (Getcwd(cwd.get(), 255 * chunks) != nullptr) + const int bufSize = 255 * chunks; + std::unique_ptr cwd(new SystemChar[bufSize]); + if (Getcwd(cwd.get(), bufSize) != nullptr) { return SystemString(cwd.get()); + } if (errno != ERANGE) { // It's not ERANGE, so we don't know how to handle it LogModule.report(logvisor::Fatal, fmt("Cannot determine the current path.")); @@ -593,9 +596,9 @@ int RecursiveMakeDir(const SystemChar* dir) { SystemChar tmp[1024]; /* copy path */ - std::wcsncpy(tmp, dir, 1024); + std::wcsncpy(tmp, dir, std::size(tmp)); const size_t len = std::wcslen(tmp); - if (len >= 1024) { + if (len >= std::size(tmp)) { return -1; } @@ -640,9 +643,9 @@ int RecursiveMakeDir(const SystemChar* dir) { SystemChar tmp[1024]; /* copy path */ - std::strncpy(tmp, dir, 1024); + std::strncpy(tmp, dir, std::size(tmp)); const size_t len = std::strlen(tmp); - if (len >= 1024) { + if (len >= std::size(tmp)) { return -1; } From e5a0d657b3d4592f4abe8a615557b74427b787cc Mon Sep 17 00:00:00 2001 From: Lioncash Date: Sat, 24 Aug 2019 16:04:28 -0400 Subject: [PATCH 7/8] hecl/hecl: Remove pointer casts from GetTmpDir() We can just make the pointers point to const data, eliminating the need to cast away const. --- hecl/lib/hecl.cpp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/hecl/lib/hecl.cpp b/hecl/lib/hecl.cpp index 5808c38bf..05bb5268a 100644 --- a/hecl/lib/hecl.cpp +++ b/hecl/lib/hecl.cpp @@ -690,16 +690,16 @@ int RecursiveMakeDir(const SystemChar* dir) { const SystemChar* GetTmpDir() { #ifdef _WIN32 #if WINDOWS_STORE - wchar_t* TMPDIR = nullptr; + const wchar_t* TMPDIR = nullptr; #else - wchar_t* TMPDIR = _wgetenv(L"TEMP"); + const wchar_t* TMPDIR = _wgetenv(L"TEMP"); if (!TMPDIR) - TMPDIR = (wchar_t*)L"\\Temp"; + TMPDIR = L"\\Temp"; #endif #else - char* TMPDIR = getenv("TMPDIR"); + const char* TMPDIR = getenv("TMPDIR"); if (!TMPDIR) - TMPDIR = (char*)"/tmp"; + TMPDIR = "/tmp"; #endif return TMPDIR; } From 6968f8a301a0f856dbde8537d276bfc6c8ab4bd5 Mon Sep 17 00:00:00 2001 From: Lioncash Date: Sat, 24 Aug 2019 16:14:37 -0400 Subject: [PATCH 8/8] hecl/hecl: Use nullptr where applicable --- hecl/lib/hecl.cpp | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/hecl/lib/hecl.cpp b/hecl/lib/hecl.cpp index 05bb5268a..27e491986 100644 --- a/hecl/lib/hecl.cpp +++ b/hecl/lib/hecl.cpp @@ -707,13 +707,15 @@ const SystemChar* GetTmpDir() { #if !WINDOWS_STORE int RunProcess(const SystemChar* path, const SystemChar* const args[]) { #ifdef _WIN32 - SECURITY_ATTRIBUTES sattrs = {sizeof(SECURITY_ATTRIBUTES), NULL, TRUE}; - HANDLE consoleOutReadTmp, consoleOutWrite, consoleErrWrite, consoleOutRead; + SECURITY_ATTRIBUTES sattrs = {sizeof(SECURITY_ATTRIBUTES), nullptr, TRUE}; + HANDLE consoleOutReadTmp = INVALID_HANDLE_VALUE; + HANDLE consoleOutWrite = INVALID_HANDLE_VALUE; if (!CreatePipe(&consoleOutReadTmp, &consoleOutWrite, &sattrs, 0)) { LogModule.report(logvisor::Fatal, fmt("Error with CreatePipe")); return -1; } + HANDLE consoleErrWrite = INVALID_HANDLE_VALUE; if (!DuplicateHandle(GetCurrentProcess(), consoleOutWrite, GetCurrentProcess(), &consoleErrWrite, 0, TRUE, DUPLICATE_SAME_ACCESS)) { LogModule.report(logvisor::Fatal, fmt("Error with DuplicateHandle")); @@ -722,11 +724,12 @@ int RunProcess(const SystemChar* path, const SystemChar* const args[]) { return -1; } + HANDLE consoleOutRead = INVALID_HANDLE_VALUE; if (!DuplicateHandle(GetCurrentProcess(), consoleOutReadTmp, GetCurrentProcess(), &consoleOutRead, // Address of new handle. 0, FALSE, // Make it uninheritable. DUPLICATE_SAME_ACCESS)) { - LogModule.report(logvisor::Fatal, fmt("Error with DupliateHandle")); + LogModule.report(logvisor::Fatal, fmt("Error with DuplicateHandle")); CloseHandle(consoleOutReadTmp); CloseHandle(consoleOutWrite); CloseHandle(consoleErrWrite); @@ -745,18 +748,18 @@ int RunProcess(const SystemChar* path, const SystemChar* const args[]) { STARTUPINFO sinfo = {sizeof(STARTUPINFO)}; HANDLE nulHandle = CreateFileW(L"nul", GENERIC_WRITE, FILE_SHARE_READ | FILE_SHARE_WRITE, &sattrs, OPEN_EXISTING, - FILE_ATTRIBUTE_NORMAL, NULL); + FILE_ATTRIBUTE_NORMAL, nullptr); sinfo.dwFlags = STARTF_USESTDHANDLES; sinfo.hStdInput = nulHandle; sinfo.hStdError = consoleErrWrite; sinfo.hStdOutput = consoleOutWrite; PROCESS_INFORMATION pinfo = {}; - if (!CreateProcessW(path, (LPWSTR)cmdLine.c_str(), NULL, NULL, TRUE, NORMAL_PRIORITY_CLASS, NULL, NULL, &sinfo, + if (!CreateProcessW(path, cmdLine.data(), nullptr, nullptr, TRUE, NORMAL_PRIORITY_CLASS, nullptr, nullptr, &sinfo, &pinfo)) { LPWSTR messageBuffer = nullptr; - FormatMessageW(FORMAT_MESSAGE_ALLOCATE_BUFFER | FORMAT_MESSAGE_FROM_SYSTEM | FORMAT_MESSAGE_IGNORE_INSERTS, NULL, - GetLastError(), MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT), (LPWSTR)&messageBuffer, 0, NULL); + FormatMessageW(FORMAT_MESSAGE_ALLOCATE_BUFFER | FORMAT_MESSAGE_FROM_SYSTEM | FORMAT_MESSAGE_IGNORE_INSERTS, nullptr, + GetLastError(), MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT), (LPWSTR)&messageBuffer, 0, nullptr); LogModule.report(logvisor::Error, fmt(L"unable to launch process from {}: {}"), path, messageBuffer); LocalFree(messageBuffer); @@ -778,7 +781,7 @@ int RunProcess(const SystemChar* path, const SystemChar* const args[]) { DWORD nCharsWritten; while (consoleThreadRunning) { - if (!ReadFile(consoleOutRead, lpBuffer, sizeof(lpBuffer), &nBytesRead, NULL) || !nBytesRead) { + if (!ReadFile(consoleOutRead, lpBuffer, sizeof(lpBuffer), &nBytesRead, nullptr) || !nBytesRead) { DWORD err = GetLastError(); if (err == ERROR_BROKEN_PIPE) break; // pipe done - normal exit path. @@ -788,7 +791,7 @@ int RunProcess(const SystemChar* path, const SystemChar* const args[]) { // Display the character read on the screen. auto lk = logvisor::LockLog(); - if (!WriteConsoleA(GetStdHandle(STD_OUTPUT_HANDLE), lpBuffer, nBytesRead, &nCharsWritten, NULL)) { + if (!WriteConsoleA(GetStdHandle(STD_OUTPUT_HANDLE), lpBuffer, nBytesRead, &nCharsWritten, nullptr)) { // LogModule.report(logvisor::Error, fmt("Error with WriteConsole: {:08X}"), GetLastError()); } }