From fa98beb2212e3e4e0dfa939dc301615ee9258650 Mon Sep 17 00:00:00 2001 From: Chris Hennes Date: Tue, 26 Aug 2025 19:09:33 -0500 Subject: [PATCH] App: refactor to use unique_ptr Also address other reviewer comments. --- src/App/Application.cpp | 11 ++- src/App/Application.h | 4 +- src/App/ApplicationDirectories.cpp | 21 +++--- src/App/ApplicationDirectories.h | 8 ++- tests/src/App/ApplicationDirectories.cpp | 88 +++++++++++++++++++++--- 5 files changed, 104 insertions(+), 28 deletions(-) diff --git a/src/App/Application.cpp b/src/App/Application.cpp index b25138a854..b69d139dc2 100644 --- a/src/App/Application.cpp +++ b/src/App/Application.cpp @@ -186,7 +186,7 @@ Base::ConsoleObserverStd *Application::_pConsoleObserverStd = nullptr; Base::ConsoleObserverFile *Application::_pConsoleObserverFile = nullptr; AppExport std::map Application::mConfig; -std::shared_ptr Application::_appDirs; +std::unique_ptr Application::_appDirs; //************************************************************************** @@ -1142,7 +1142,7 @@ bool Application::isDevelopmentVersion() return suffix == "dev"; } -std::shared_ptr Application::directories() { +const std::unique_ptr& Application::directories() { return _appDirs; } @@ -1153,10 +1153,7 @@ std::string Application::getTempPath() std::string Application::getTempFileName(const char* FileName) { - if (FileName) { - return Base::FileInfo::pathToString(_appDirs->getTempFileName(FileName)); - } - return Base::FileInfo::pathToString(_appDirs->getTempFileName(std::string())); + return Base::FileInfo::pathToString(_appDirs->getTempFileName(FileName ? FileName : std::string())); } std::string Application::getUserCachePath() @@ -2573,7 +2570,7 @@ void Application::initConfig(int argc, char ** argv) } // extract home paths - _appDirs = std::make_shared(mConfig); + _appDirs = std::make_unique(mConfig); if (vm.contains("safe-mode")) { SafeMode::StartSafeMode(); diff --git a/src/App/Application.h b/src/App/Application.h index 6abc36aa4a..b1f482adc9 100644 --- a/src/App/Application.h +++ b/src/App/Application.h @@ -426,7 +426,7 @@ public: static bool isDevelopmentVersion(); /// Access to the various directories for the program a replacement for the get*Path methods below - static std::shared_ptr directories(); + static const std::unique_ptr& directories(); /*! Returns the temporary directory. By default, this is set to the @@ -632,7 +632,7 @@ private: /// startup configuration container static std::map mConfig; /// Management of and access to applications directories - static std::shared_ptr _appDirs; + static std::unique_ptr _appDirs; static int _argc; static char ** _argv; //@} diff --git a/src/App/ApplicationDirectories.cpp b/src/App/ApplicationDirectories.cpp index eb5305a529..011b37d8a8 100644 --- a/src/App/ApplicationDirectories.cpp +++ b/src/App/ApplicationDirectories.cpp @@ -56,13 +56,7 @@ fs::path qstringToPath(const QString& path) ApplicationDirectories::ApplicationDirectories(std::map &config) { - try { - int major = std::stoi(config.at("BuildVersionMajor")); - int minor = std::stoi(config.at("BuildVersionMinor")); - _currentVersion = std::make_tuple(major, minor); - } catch (const std::exception& e) { - throw Base::RuntimeError("Failed to parse version from config: " + std::string(e.what())); - } + _currentVersion = extractVersionFromConfigMap(config); configurePaths(config); configureResourceDirectory(config); configureLibraryDirectory(config); @@ -724,5 +718,16 @@ fs::path ApplicationDirectories::findHomePath(const char* sCall) } #else -# error "std::string ApplicationDirectories::FindHomePath(const char*) not implemented" +# error "std::string ApplicationDirectories::findHomePath(const char*) not implemented" #endif + +std::tuple ApplicationDirectories::extractVersionFromConfigMap(const std::map &config) +{ + try { + int major = std::stoi(config.at("BuildVersionMajor")); + int minor = std::stoi(config.at("BuildVersionMinor")); + return std::make_tuple(major, minor); + } catch (const std::exception& e) { + throw Base::RuntimeError("Failed to parse version from config: " + std::string(e.what())); + } +} diff --git a/src/App/ApplicationDirectories.h b/src/App/ApplicationDirectories.h index 55ff7197b5..87f86df7b7 100644 --- a/src/App/ApplicationDirectories.h +++ b/src/App/ApplicationDirectories.h @@ -193,7 +193,6 @@ namespace App { void configureHelpDirectory(const std::map& mConfig); /*! - * \brief getCustomPaths * Returns a tuple of path names where to store config, data, and temp. files. * The method therefore reads the environment variables: * \list @@ -205,7 +204,6 @@ namespace App { static std::tuple getCustomPaths(); /*! - * \brief getStandardPaths * Returns a tuple of XDG-compliant standard paths names where to store config, data and cached files. * The method therefore reads the environment variables: * \list @@ -216,6 +214,12 @@ namespace App { */ std::tuple getStandardPaths(); + /// Find the BuildVersionMajor, BuildVersionMinor pair in the config map, convert them to an int tuple, and + /// return it. If the pair is not found, or cannot be converted to integers, a RuntimeError is raised. + /// \param config The config map to search. + /// \return The version tuple. + static std::tuple extractVersionFromConfigMap(const std::map &config); + private: std::tuple _currentVersion; std::filesystem::path _home; diff --git a/tests/src/App/ApplicationDirectories.cpp b/tests/src/App/ApplicationDirectories.cpp index dff98381a0..493c1348e1 100644 --- a/tests/src/App/ApplicationDirectories.cpp +++ b/tests/src/App/ApplicationDirectories.cpp @@ -46,6 +46,12 @@ public: { appendVersionIfPossible(basePath, subdirs); } + + std::tuple + wrapExtractVersionFromConfigMap(const std::map& config) + { + return extractVersionFromConfigMap(config); + } }; class ApplicationDirectoriesTest: public ::testing::Test @@ -70,11 +76,11 @@ protected: return config; } - std::shared_ptr makeAppDirsForVersion(int major, int minor) + std::unique_ptr makeAppDirsForVersion(int major, int minor) { auto configuration = generateConfig({{"BuildVersionMajor", std::to_string(major)}, {"BuildVersionMinor", std::to_string(minor)}}); - return std::make_shared(configuration); + return std::make_unique(configuration); } fs::path makePathForVersion(const fs::path& base, int major, int minor) @@ -125,7 +131,7 @@ TEST_F(ApplicationDirectoriesTest, usingCurrentVersionConfigFalseWhenDirDoesntMa // Act: generate a directory structure with the same version auto configuration = generateConfig({{"BuildVersionMajor", std::to_string(major + 1)}, {"BuildVersionMinor", std::to_string(minor)}}); - auto appDirs = std::make_shared(configuration); + auto appDirs = std::make_unique(configuration); // Assert EXPECT_FALSE(appDirs->usingCurrentVersionConfig(testPath)); @@ -582,7 +588,7 @@ TEST_F(ApplicationDirectoriesTest, migrateAllPathsProcessesMultipleInputs) } // Already versioned (final component is a version) -> no change -TEST_F(ApplicationDirectoriesTest, AppendVec_AlreadyVersioned_Bails) +TEST_F(ApplicationDirectoriesTest, appendVecAlreadyVersionedBails) { auto appDirs = makeAppDirsForVersion(5, 4); @@ -598,7 +604,7 @@ TEST_F(ApplicationDirectoriesTest, AppendVec_AlreadyVersioned_Bails) } // Base exists & current version dir present -> append current -TEST_F(ApplicationDirectoriesTest, AppendVec_BaseExists_AppendsCurrentWhenPresent) +TEST_F(ApplicationDirectoriesTest, appendVecBaseExistsAppendsCurrentWhenPresent) { auto appDirs = makeAppDirsForVersion(5, 4); @@ -614,7 +620,7 @@ TEST_F(ApplicationDirectoriesTest, AppendVec_BaseExists_AppendsCurrentWhenPresen } // Base exists, no current; lower minors exist -> append highest ≤ current in same major -TEST_F(ApplicationDirectoriesTest, AppendVec_PicksHighestLowerMinorInSameMajor) +TEST_F(ApplicationDirectoriesTest, appendVecPicksHighestLowerMinorInSameMajor) { auto appDirs = makeAppDirsForVersion(5, 4); @@ -632,7 +638,7 @@ TEST_F(ApplicationDirectoriesTest, AppendVec_PicksHighestLowerMinorInSameMajor) } // Base exists, nothing in current major; lower major exists -> append highest available lower major -TEST_F(ApplicationDirectoriesTest, AppendVec_FallsBackToLowerMajor) +TEST_F(ApplicationDirectoriesTest, appendVecFallsBackToLowerMajor) { auto appDirs = makeAppDirsForVersion(5, 4); @@ -648,7 +654,7 @@ TEST_F(ApplicationDirectoriesTest, AppendVec_FallsBackToLowerMajor) } // Base exists but contains no versioned subdirs -> append nothing (vector unchanged) -TEST_F(ApplicationDirectoriesTest, AppendVec_NoVersionedChildren_LeavesVectorUnchanged) +TEST_F(ApplicationDirectoriesTest, appendVecNoVersionedChildrenLeavesVectorUnchanged) { auto appDirs = makeAppDirsForVersion(5, 4); @@ -662,7 +668,7 @@ TEST_F(ApplicationDirectoriesTest, AppendVec_NoVersionedChildren_LeavesVectorUnc } // Base does not exist -> append current version string -TEST_F(ApplicationDirectoriesTest, AppendVec_BaseMissing_AppendsCurrentSuffix) +TEST_F(ApplicationDirectoriesTest, appendVecBaseMissingAppendsCurrentSuffix) { auto appDirs = makeAppDirsForVersion(5, 4); @@ -675,6 +681,70 @@ TEST_F(ApplicationDirectoriesTest, AppendVec_BaseMissing_AppendsCurrentSuffix) EXPECT_EQ(sub.back(), App::ApplicationDirectories::versionStringForPath(5, 4)); } +// Happy path: exact integers +TEST_F(ApplicationDirectoriesTest, extractVersionSucceedsWithPlainIntegers) +{ + auto appDirs = makeAppDirsForVersion(5, 4); + std::map m {{"BuildVersionMajor", "7"}, {"BuildVersionMinor", "2"}}; + auto [maj, min] = appDirs->wrapExtractVersionFromConfigMap(m); + EXPECT_EQ(maj, 7); + EXPECT_EQ(min, 2); +} + +// Whitespace tolerated by std::stoi +TEST_F(ApplicationDirectoriesTest, extractVersionSucceedsWithWhitespace) +{ + auto appDirs = makeAppDirsForVersion(5, 4); + std::map m {{"BuildVersionMajor", " 10 "}, + {"BuildVersionMinor", "\t3\n"}}; + auto [maj, min] = appDirs->wrapExtractVersionFromConfigMap(m); + EXPECT_EQ(maj, 10); + EXPECT_EQ(min, 3); +} + +// Missing major key -> rethrows as Base::RuntimeError +TEST_F(ApplicationDirectoriesTest, extractVersionMissingMajorThrowsRuntimeError) +{ + auto appDirs = makeAppDirsForVersion(5, 4); + std::map m {{"BuildVersionMinor", "1"}}; + EXPECT_THROW(appDirs->wrapExtractVersionFromConfigMap(m), Base::RuntimeError); +} + +// Missing minor key -> rethrows as Base::RuntimeError +TEST_F(ApplicationDirectoriesTest, extractVersionMissingMinorThrowsRuntimeError) +{ + auto appDirs = makeAppDirsForVersion(5, 4); + std::map m {{"BuildVersionMajor", "1"}}; + EXPECT_THROW(appDirs->wrapExtractVersionFromConfigMap(m), Base::RuntimeError); +} + +// Non-numeric -> std::stoi throws invalid_argument, rethrown as Base::RuntimeError +TEST_F(ApplicationDirectoriesTest, extractVersionNonNumericThrowsRuntimeError) +{ + auto appDirs = makeAppDirsForVersion(5, 4); + std::map m {{"BuildVersionMajor", "abc"}, {"BuildVersionMinor", "2"}}; + EXPECT_THROW(appDirs->wrapExtractVersionFromConfigMap(m), Base::RuntimeError); +} + +// Overflow -> std::stoi throws out_of_range, rethrown as Base::RuntimeError +TEST_F(ApplicationDirectoriesTest, extractVersionOverflowThrowsRuntimeError) +{ + auto appDirs = makeAppDirsForVersion(5, 4); + std::map m {{"BuildVersionMajor", "9999999999999999999999999"}, + {"BuildVersionMinor", "1"}}; + EXPECT_THROW(appDirs->wrapExtractVersionFromConfigMap(m), Base::RuntimeError); +} + +// Document current behavior: negative numbers are accepted and returned as-is +TEST_F(ApplicationDirectoriesTest, extractVersionNegativeNumbersPassThrough) +{ + auto appDirs = makeAppDirsForVersion(5, 4); + std::map m {{"BuildVersionMajor", "-2"}, {"BuildVersionMinor", "-7"}}; + auto [maj, min] = appDirs->wrapExtractVersionFromConfigMap(m); + EXPECT_EQ(maj, -2); + EXPECT_EQ(min, -7); +} + /* NOLINTEND( readability-magic-numbers, cppcoreguidelines-avoid-magic-numbers,