From 6ebe89c6b3b3c130b8c28956c1ba566092868bac Mon Sep 17 00:00:00 2001 From: David Fields Date: Fri, 30 Jul 2021 11:54:37 -0700 Subject: [PATCH 1/3] Use throw rather than abort() for guid parse failures Make `winrt::guid("...")` more useful at runtime by throwing on failure instead of aborting the program. --- strings/base_types.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/strings/base_types.h b/strings/base_types.h index 751b949b3..d88ea32ec 100644 --- a/strings/base_types.h +++ b/strings/base_types.h @@ -89,7 +89,7 @@ WINRT_EXPORT namespace winrt { if (value.size() != 36 || value[8] != '-' || value[13] != '-' || value[18] != '-' || value[23] != '-') { - abort(); + throw std::invalid_argument("value is not a valid GUID string"); } return From f2c9ab5307b91ac7137707a2fa0d8b44437c3aaf Mon Sep 17 00:00:00 2001 From: "David Fields (ANALOG)" Date: Fri, 30 Jul 2021 15:09:08 -0700 Subject: [PATCH 2/3] Remove noexcept, add tests --- strings/base_types.h | 31 +++++++++++++++++++++---------- test/test/guid.cpp | 16 ++++++++++++++++ test/test/test.vcxproj | 1 + 3 files changed, 38 insertions(+), 10 deletions(-) create mode 100644 test/test/guid.cpp diff --git a/strings/base_types.h b/strings/base_types.h index d88ea32ec..b4c994b34 100644 --- a/strings/base_types.h +++ b/strings/base_types.h @@ -20,7 +20,7 @@ namespace winrt::impl }; template - constexpr uint8_t hex_to_uint(T const c) noexcept + constexpr uint8_t hex_to_uint(T const c) { if (c >= '0' && c <= '9') { @@ -36,22 +36,22 @@ namespace winrt::impl } else { - abort(); + throw std::invalid_argument("Character is not a hexadecimal digit"); } } template - constexpr uint8_t hex_to_uint8(T const a, T const b) noexcept + constexpr uint8_t hex_to_uint8(T const a, T const b) { return (hex_to_uint(a) << 4) | hex_to_uint(b); } - constexpr uint16_t uint8_to_uint16(uint8_t a, uint8_t b) noexcept + constexpr uint16_t uint8_to_uint16(uint8_t a, uint8_t b) { return (static_cast(a) << 8) | static_cast(b); } - constexpr uint32_t uint8_to_uint32(uint8_t a, uint8_t b, uint8_t c, uint8_t d) noexcept + constexpr uint32_t uint8_to_uint32(uint8_t a, uint8_t b, uint8_t c, uint8_t d) { return (static_cast(uint8_to_uint16(a, b)) << 16) | static_cast(uint8_to_uint16(c, d)); @@ -85,7 +85,7 @@ WINRT_EXPORT namespace winrt private: template - static constexpr guid parse(TStringView const value) noexcept + static constexpr guid parse(TStringView const value) { if (value.size() != 36 || value[8] != '-' || value[13] != '-' || value[18] != '-' || value[23] != '-') { @@ -170,16 +170,27 @@ WINRT_EXPORT namespace winrt } }; - inline bool operator==(guid const& left, guid const& right) noexcept + constexpr bool operator==(guid const& left, guid const& right) noexcept { - return !memcmp(&left, &right, sizeof(left)); + return + (left.Data1 == right.Data1) && + (left.Data2 == right.Data2) && + (left.Data3 == right.Data3) && + (left.Data4[0] == right.Data4[0]) && + (left.Data4[1] == right.Data4[1]) && + (left.Data4[2] == right.Data4[2]) && + (left.Data4[3] == right.Data4[3]) && + (left.Data4[4] == right.Data4[4]) && + (left.Data4[5] == right.Data4[5]) && + (left.Data4[6] == right.Data4[6]) && + (left.Data4[7] == right.Data4[7]); } - inline bool operator!=(guid const& left, guid const& right) noexcept + constexpr bool operator!=(guid const& left, guid const& right) noexcept { return !(left == right); } - + inline bool operator<(guid const& left, guid const& right) noexcept { return memcmp(&left, &right, sizeof(left)) < 0; diff --git a/test/test/guid.cpp b/test/test/guid.cpp new file mode 100644 index 000000000..0ee9b5a40 --- /dev/null +++ b/test/test/guid.cpp @@ -0,0 +1,16 @@ +#include "pch.h" + +TEST_CASE("guid") +{ + STATIC_REQUIRE(winrt::guid("00112233-4455-6677-8899-aabbccddeeff") == winrt::guid(0x00112233, 0x4455, 0x6677, { 0x88, 0x99, 0xaa, 0xbb, 0xcc, 0xdd, 0xee, 0xff })); + REQUIRE(winrt::guid("00112233-4455-6677-8899-aabbccddeeff") == winrt::guid(0x00112233, 0x4455, 0x6677, { 0x88, 0x99, 0xaa, 0xbb, 0xcc, 0xdd, 0xee, 0xff })); + constexpr char* null = nullptr; + REQUIRE_THROWS_AS(winrt::guid(null), std::invalid_argument); + REQUIRE_THROWS_AS(winrt::guid(""), std::invalid_argument); + REQUIRE_THROWS_AS(winrt::guid("not a guid"), std::invalid_argument); + REQUIRE_THROWS_AS(winrt::guid("same length string that's not a guid"), std::invalid_argument); + REQUIRE_THROWS_AS(winrt::guid("too long string that's also not a guid"), std::invalid_argument); + REQUIRE_THROWS_AS(winrt::guid("00112233-4455-6677-8899-aabbccddeeff with extra"), std::invalid_argument); + REQUIRE_THROWS_AS(winrt::guid("xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx"), std::invalid_argument); + REQUIRE_THROWS_AS(winrt::guid("{00112233-4455-6677-8899-aabbccddeeff}"), std::invalid_argument); +} \ No newline at end of file diff --git a/test/test/test.vcxproj b/test/test/test.vcxproj index 3d9ad5f40..ed6e934bd 100644 --- a/test/test/test.vcxproj +++ b/test/test/test.vcxproj @@ -335,6 +335,7 @@ + From f890d904a255a9b9433425bef774a0600b6f8cdf Mon Sep 17 00:00:00 2001 From: "David Fields (ANALOG)" Date: Fri, 30 Jul 2021 15:43:56 -0700 Subject: [PATCH 3/3] Revert operator== changes, revise tests --- strings/base_types.h | 17 +++-------------- test/test/guid.cpp | 21 +++++++++++++++++---- 2 files changed, 20 insertions(+), 18 deletions(-) diff --git a/strings/base_types.h b/strings/base_types.h index b4c994b34..70e97fbb7 100644 --- a/strings/base_types.h +++ b/strings/base_types.h @@ -170,23 +170,12 @@ WINRT_EXPORT namespace winrt } }; - constexpr bool operator==(guid const& left, guid const& right) noexcept + inline bool operator==(guid const& left, guid const& right) noexcept { - return - (left.Data1 == right.Data1) && - (left.Data2 == right.Data2) && - (left.Data3 == right.Data3) && - (left.Data4[0] == right.Data4[0]) && - (left.Data4[1] == right.Data4[1]) && - (left.Data4[2] == right.Data4[2]) && - (left.Data4[3] == right.Data4[3]) && - (left.Data4[4] == right.Data4[4]) && - (left.Data4[5] == right.Data4[5]) && - (left.Data4[6] == right.Data4[6]) && - (left.Data4[7] == right.Data4[7]); + return !memcmp(&left, &right, sizeof(left)); } - constexpr bool operator!=(guid const& left, guid const& right) noexcept + inline bool operator!=(guid const& left, guid const& right) noexcept { return !(left == right); } diff --git a/test/test/guid.cpp b/test/test/guid.cpp index 0ee9b5a40..b3edbc570 100644 --- a/test/test/guid.cpp +++ b/test/test/guid.cpp @@ -2,10 +2,23 @@ TEST_CASE("guid") { - STATIC_REQUIRE(winrt::guid("00112233-4455-6677-8899-aabbccddeeff") == winrt::guid(0x00112233, 0x4455, 0x6677, { 0x88, 0x99, 0xaa, 0xbb, 0xcc, 0xdd, 0xee, 0xff })); - REQUIRE(winrt::guid("00112233-4455-6677-8899-aabbccddeeff") == winrt::guid(0x00112233, 0x4455, 0x6677, { 0x88, 0x99, 0xaa, 0xbb, 0xcc, 0xdd, 0xee, 0xff })); - constexpr char* null = nullptr; - REQUIRE_THROWS_AS(winrt::guid(null), std::invalid_argument); + constexpr winrt::guid expected{ 0x00112233, 0x4455, 0x6677, { 0x88, 0x99, 0xaa, 0xbb, 0xcc, 0xdd, 0xee, 0xff } }; + + STATIC_REQUIRE(winrt::guid("00112233-4455-6677-8899-aabbccddeeff").Data1 == expected.Data1); + STATIC_REQUIRE(winrt::guid("00112233-4455-6677-8899-aabbccddeeff").Data2 == expected.Data2); + STATIC_REQUIRE(winrt::guid("00112233-4455-6677-8899-aabbccddeeff").Data3 == expected.Data3); + STATIC_REQUIRE(winrt::guid("00112233-4455-6677-8899-aabbccddeeff").Data4[0] == expected.Data4[0]); + STATIC_REQUIRE(winrt::guid("00112233-4455-6677-8899-aabbccddeeff").Data4[1] == expected.Data4[1]); + STATIC_REQUIRE(winrt::guid("00112233-4455-6677-8899-aabbccddeeff").Data4[2] == expected.Data4[2]); + STATIC_REQUIRE(winrt::guid("00112233-4455-6677-8899-aabbccddeeff").Data4[3] == expected.Data4[3]); + STATIC_REQUIRE(winrt::guid("00112233-4455-6677-8899-aabbccddeeff").Data4[4] == expected.Data4[4]); + STATIC_REQUIRE(winrt::guid("00112233-4455-6677-8899-aabbccddeeff").Data4[5] == expected.Data4[5]); + STATIC_REQUIRE(winrt::guid("00112233-4455-6677-8899-aabbccddeeff").Data4[6] == expected.Data4[6]); + STATIC_REQUIRE(winrt::guid("00112233-4455-6677-8899-aabbccddeeff").Data4[7] == expected.Data4[7]); + + REQUIRE(winrt::guid("00112233-4455-6677-8899-aabbccddeeff") == expected); + REQUIRE(winrt::guid({ "{00112233-4455-6677-8899-aabbccddeeff}" + 1, 36 }) == expected); + REQUIRE_THROWS_AS(winrt::guid(""), std::invalid_argument); REQUIRE_THROWS_AS(winrt::guid("not a guid"), std::invalid_argument); REQUIRE_THROWS_AS(winrt::guid("same length string that's not a guid"), std::invalid_argument);