From a47a53c893165b7cf8741a8a4af5aac5336d6ad3 Mon Sep 17 00:00:00 2001 From: Michael Kitzan Date: Tue, 3 Dec 2019 11:05:41 -0800 Subject: [PATCH] GetGuid noexcept Fix (#3806) ## Summary of the Pull Request Fixed the noexcept specifier on `GetGuid`, and corrected `FindProfile` and `FindGuid` so they don't throw. Also, adjusted `SettingsTests` to reflect these changes. ## References ## PR Checklist * [x] Closes #3763 * [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA * [x] Tests added/passed updated a test group in `SettingsTests` * [ ] Requires documentation to be updated * [x] I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #3763 ## Detailed Description of the Pull Request / Additional comments The `noexcept` specifier on `GetGuid` was not removed when `Profile` was [updated](https://github.com/microsoft/terminal/issues/3763#issuecomment-559497094) to `std::optional`. This PR fixes that and modifies two helper functions `FindProfile` and `FindGuid` in `CascadiaSettings` to work correctly if `GetGuid` does throw. ## Validation Steps Performed Updated the `TestHelperFunctions` test group in `SettingsTests` and made sure the tests pass. --- .../LocalTests_TerminalApp/SettingsTests.cpp | 19 +++++++++---------- src/cascadia/TerminalApp/CascadiaSettings.cpp | 15 ++++++++++++--- src/cascadia/TerminalApp/Profile.cpp | 2 +- src/cascadia/TerminalApp/Profile.h | 2 +- 4 files changed, 23 insertions(+), 15 deletions(-) diff --git a/src/cascadia/LocalTests_TerminalApp/SettingsTests.cpp b/src/cascadia/LocalTests_TerminalApp/SettingsTests.cpp index d243f7517a..a2c2cf26d8 100644 --- a/src/cascadia/LocalTests_TerminalApp/SettingsTests.cpp +++ b/src/cascadia/LocalTests_TerminalApp/SettingsTests.cpp @@ -1280,11 +1280,11 @@ namespace TerminalAppLocalTests "guid": "{6239a42c-6666-49a3-80bd-e8fdd045185c}" }, { - "name" : "Ubuntu", - "guid" : "{2C4DE342-38B7-51CF-B940-2309A097F518}" + "name" : "ThisProfileShouldNotThrow" }, { - "name" : "ThisProfileShouldNotCrash" + "name" : "Ubuntu", + "guid" : "{2C4DE342-38B7-51CF-B940-2309A097F518}" } ] })" }; @@ -1292,11 +1292,13 @@ namespace TerminalAppLocalTests auto name0{ L"profile0" }; auto name1{ L"profile1" }; auto name2{ L"Ubuntu" }; + auto name3{ L"ThisProfileShouldNotThrow" }; auto badName{ L"DoesNotExist" }; auto guid0{ Microsoft::Console::Utils::GuidFromString(L"{6239a42c-5555-49a3-80bd-e8fdd045185c}") }; auto guid1{ Microsoft::Console::Utils::GuidFromString(L"{6239a42c-6666-49a3-80bd-e8fdd045185c}") }; auto guid2{ Microsoft::Console::Utils::GuidFromString(L"{2C4DE342-38B7-51CF-B940-2309A097F518}") }; + auto fakeGuid{ Microsoft::Console::Utils::GuidFromString(L"{FFFFFFFF-FFFF-FFFF-FFFF-FFFFFFFFFFFF}") }; std::optional badGuid{}; VerifyParseSucceeded(settings0String); @@ -1308,22 +1310,19 @@ namespace TerminalAppLocalTests VERIFY_ARE_EQUAL(guid0, settings.FindGuid(name0)); VERIFY_ARE_EQUAL(guid1, settings.FindGuid(name1)); VERIFY_ARE_EQUAL(guid2, settings.FindGuid(name2)); + VERIFY_ARE_EQUAL(badGuid, settings.FindGuid(name3)); VERIFY_ARE_EQUAL(badGuid, settings.FindGuid(badName)); - // Following test will fail because GetGuid throws (despite being noexcept) - // VERIFY_ARE_EQUAL(badGuid, settings.FindGuid(L"ThisProfileShouldNotCrash")); - // Following lines only work because the GUID-less profile is last auto prof0{ settings.FindProfile(guid0) }; auto prof1{ settings.FindProfile(guid1) }; auto prof2{ settings.FindProfile(guid2) }; - // Following line will fail because GetGuid throws (despite being noexcept) - // auto badProf{ settings.FindProfile(badGuid) }; + + auto badProf{ settings.FindProfile(fakeGuid) }; + VERIFY_ARE_EQUAL(badProf, nullptr); VERIFY_ARE_EQUAL(name0, prof0->GetName()); VERIFY_ARE_EQUAL(name1, prof1->GetName()); VERIFY_ARE_EQUAL(name2, prof2->GetName()); - // Following test will fail because GetGuid throws (despite being noexcept) - // VERIFY_ARE_EQUAL(badName, badProf->GetName()); } void SettingsTests::TestLayerGlobalsOnRoot() diff --git a/src/cascadia/TerminalApp/CascadiaSettings.cpp b/src/cascadia/TerminalApp/CascadiaSettings.cpp index a8058267c5..629d0007c2 100644 --- a/src/cascadia/TerminalApp/CascadiaSettings.cpp +++ b/src/cascadia/TerminalApp/CascadiaSettings.cpp @@ -78,7 +78,12 @@ std::optional CascadiaSettings::FindGuid(const std::wstring& profileName) { if (profileName == profile.GetName()) { - profileGuid = profile.GetGuid(); + try + { + profileGuid = profile.GetGuid(); + } + CATCH_LOG(); + break; } } @@ -98,10 +103,14 @@ const Profile* CascadiaSettings::FindProfile(GUID profileGuid) const noexcept { for (auto& profile : _profiles) { - if (profile.GetGuid() == profileGuid) + try { - return &profile; + if (profile.GetGuid() == profileGuid) + { + return &profile; + } } + CATCH_LOG(); } return nullptr; } diff --git a/src/cascadia/TerminalApp/Profile.cpp b/src/cascadia/TerminalApp/Profile.cpp index d689bc6c7b..788e2b6db5 100644 --- a/src/cascadia/TerminalApp/Profile.cpp +++ b/src/cascadia/TerminalApp/Profile.cpp @@ -138,7 +138,7 @@ bool Profile::HasSource() const noexcept return _source.has_value(); } -GUID Profile::GetGuid() const noexcept +GUID Profile::GetGuid() const { // This can throw if we never had our guid set to a legitimate value. THROW_HR_IF_MSG(E_FAIL, !_guid.has_value(), "Profile._guid always expected to have a value"); diff --git a/src/cascadia/TerminalApp/Profile.h b/src/cascadia/TerminalApp/Profile.h index 7f42641cb4..ba85b977c2 100644 --- a/src/cascadia/TerminalApp/Profile.h +++ b/src/cascadia/TerminalApp/Profile.h @@ -64,7 +64,7 @@ public: bool HasGuid() const noexcept; bool HasSource() const noexcept; - GUID GetGuid() const noexcept; + GUID GetGuid() const; void SetSource(std::wstring_view sourceNamespace) noexcept; std::wstring_view GetName() const noexcept; bool HasConnectionType() const noexcept;