From 6a39ebf51f4321dca2251814d10e4680f6aa3bba Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tin=20=C5=A0vagelj?= Date: Sat, 25 May 2024 20:42:28 +0200 Subject: [PATCH 1/5] Move color parse message out of hex parser MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Simplifies parsing code, gives a better message and reduces responsibility of last parser to report errors. Signed-off-by: Tin Švagelj --- src/colours.cc | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/src/colours.cc b/src/colours.cc index 9adc9bf91..8f994aeaf 100644 --- a/src/colours.cc +++ b/src/colours.cc @@ -83,17 +83,13 @@ std::optional parse_hex_color(const std::string &color) { } return -1; }; - const auto none = [&]() { - NORM_ERR("can't parse hex color '%s' (%d)", name, len); - return std::nullopt; - }; uint8_t argb[4] = {0xff, 0, 0, 0}; if (len == 3 || len == 4) { bool skip_alpha = (len == 3); for (size_t i = 0; i < len; i++) { int nib = hex_nibble_value(name[i]); - if (nib < 0) { return none(); } + if (nib < 0) return std::nullopt; // Duplicate the nibble, so "#abc" -> 0xaa, 0xbb, 0xcc int val = (nib << 4) + nib; @@ -104,13 +100,13 @@ std::optional parse_hex_color(const std::string &color) { for (size_t i = 0; i + 1 < len; i += 2) { int nib1 = hex_nibble_value(name[i]); int nib2 = hex_nibble_value(name[i + 1]); - if (nib1 < 0 || nib2 < 0) { return none(); } + if (nib1 < 0 || nib2 < 0) return std::nullopt; int val = (nib1 << 4) + nib2; argb[skip_alpha + i / 2] = val; } } else { - return none(); + return std::nullopt; } return Colour(argb[1], argb[2], argb[3], argb[0]); @@ -128,6 +124,7 @@ Colour parse_color(const std::string &color) { #undef TRY_PARSER + NORM_ERR("can't parse color '%s'", color.c_str()); return ERROR_COLOUR; } From ed24c95c2a88f7d801fe0db0046afd1f6e6c51b2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tin=20=C5=A0vagelj?= Date: Sat, 25 May 2024 21:26:36 +0200 Subject: [PATCH 2/5] Do print errors in some cases MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Tin Švagelj --- src/colours.cc | 86 +++++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 74 insertions(+), 12 deletions(-) diff --git a/src/colours.cc b/src/colours.cc index 8f994aeaf..fa66afd40 100644 --- a/src/colours.cc +++ b/src/colours.cc @@ -31,7 +31,11 @@ #include "logging.h" +#include +#include #include +#include +#include Colour Colour::from_argb32(uint32_t argb) { Colour out; @@ -54,17 +58,63 @@ Colour Colour::from_argb32(uint32_t argb) { #include "colour-names-stub.hh" #endif /* BUILD_COLOUR_NAME_MAP */ -std::optional parse_color_name(const std::string &name) { - const rgb *value = color_name_hash::in_word_set(name.c_str(), name.length()); +/// Parsing result can be either a `Colour`, a none variant, or an error +/// message. +class parse_result { + std::variant value; + + public: + parse_result(Colour c) : value(c) {} + parse_result() : value(std::monostate{}) {} + parse_result(std::string msg) : value(std::move(msg)) {} + + static parse_result error(const char *format...) { + va_list args; + va_start(args, format); + size_t len = snprintf(nullptr, 0, format, args); + va_end(args); + + char *buffer = new char[len + 1]; + va_start(args, format); + snprintf(buffer, len, format, args); + va_end(args); + + auto value = std::string(buffer); + delete[] buffer; + return parse_result(value); + } - if (value == nullptr) { - return std::nullopt; - } else { + const bool has_colour() const { + return std::holds_alternative(value); + } + const std::optional get_colour() const { + if (std::holds_alternative(value)) { + return std::get(std::move(value)); + } else { + return std::nullopt; + } + } + const std::optional get_error() const { + if (std::holds_alternative(value)) { + return std::get(std::move(value)); + } else { + return std::nullopt; + } + } +}; + +parse_result none() { return parse_result(); } + +parse_result parse_color_name(const std::string &name) { + const rgb *value = color_name_hash::in_word_set(name.c_str(), name.length()); + if (value != nullptr) { return Colour{value->red, value->green, value->blue}; + } else { + return none(); } } -std::optional parse_hex_color(const std::string &color) { +parse_result parse_hex_color(const std::string &color) { const char *name = color.c_str(); size_t len = color.length(); // Skip a leading '#' if present. @@ -89,7 +139,9 @@ std::optional parse_hex_color(const std::string &color) { bool skip_alpha = (len == 3); for (size_t i = 0; i < len; i++) { int nib = hex_nibble_value(name[i]); - if (nib < 0) return std::nullopt; + if (nib < 0) + return parse_result::error("invalid hex color: '%s' (%d)", + color.c_str(), color.length()); // Duplicate the nibble, so "#abc" -> 0xaa, 0xbb, 0xcc int val = (nib << 4) + nib; @@ -100,13 +152,15 @@ std::optional parse_hex_color(const std::string &color) { for (size_t i = 0; i + 1 < len; i += 2) { int nib1 = hex_nibble_value(name[i]); int nib2 = hex_nibble_value(name[i + 1]); - if (nib1 < 0 || nib2 < 0) return std::nullopt; + if (nib1 < 0 || nib2 < 0) + return parse_result::error("invalid hex color: '%s' (%d)", + color.c_str(), color.length()); int val = (nib1 << 4) + nib2; argb[skip_alpha + i / 2] = val; } } else { - return std::nullopt; + return none(); } return Colour(argb[1], argb[2], argb[3], argb[0]); @@ -115,9 +169,17 @@ std::optional parse_hex_color(const std::string &color) { Colour parse_color(const std::string &color) { std::optional result; -#define TRY_PARSER(name) \ - std::optional value_##name = name(color); \ - if (value_##name.has_value()) { return value_##name.value(); } +#define TRY_PARSER(name) \ + parse_result value_##name = name(color); \ + if (value_##name.has_colour()) { \ + return value_##name.get_colour().value(); \ + } else { \ + auto err = value_##name.get_error(); \ + if (err.has_value()) { \ + NORM_ERR(err.value().c_str()); \ + return ERROR_COLOUR; \ + } \ + } TRY_PARSER(parse_color_name) TRY_PARSER(parse_hex_color) From e846e277761abce4b255b3f5da6d0593c02b7a68 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tin=20=C5=A0vagelj?= Date: Sat, 25 May 2024 21:35:33 +0200 Subject: [PATCH 3/5] Simplify error handling code MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Tin Švagelj --- src/colours.cc | 93 ++++++++++++++------------------------------------ 1 file changed, 26 insertions(+), 67 deletions(-) diff --git a/src/colours.cc b/src/colours.cc index fa66afd40..11e748798 100644 --- a/src/colours.cc +++ b/src/colours.cc @@ -34,8 +34,6 @@ #include #include #include -#include -#include Colour Colour::from_argb32(uint32_t argb) { Colour out; @@ -58,63 +56,36 @@ Colour Colour::from_argb32(uint32_t argb) { #include "colour-names-stub.hh" #endif /* BUILD_COLOUR_NAME_MAP */ -/// Parsing result can be either a `Colour`, a none variant, or an error -/// message. -class parse_result { - std::variant value; - - public: - parse_result(Colour c) : value(c) {} - parse_result() : value(std::monostate{}) {} - parse_result(std::string msg) : value(std::move(msg)) {} - - static parse_result error(const char *format...) { - va_list args; - va_start(args, format); - size_t len = snprintf(nullptr, 0, format, args); - va_end(args); - - char *buffer = new char[len + 1]; - va_start(args, format); - snprintf(buffer, len, format, args); - va_end(args); - - auto value = std::string(buffer); - delete[] buffer; - return parse_result(value); - } +std::optional inline no_colour() { return std::nullopt; } +std::optional parse_error(const std::string &color_str, + const char *format...) { + va_list args; + va_start(args, format); + size_t len = snprintf(nullptr, 0, format, args); + va_end(args); - const bool has_colour() const { - return std::holds_alternative(value); - } - const std::optional get_colour() const { - if (std::holds_alternative(value)) { - return std::get(std::move(value)); - } else { - return std::nullopt; - } - } - const std::optional get_error() const { - if (std::holds_alternative(value)) { - return std::get(std::move(value)); - } else { - return std::nullopt; - } - } -}; + char *reason = new char[len + 1]; + va_start(args, format); + snprintf(reason, len, format, args); + va_end(args); -parse_result none() { return parse_result(); } + CRIT_ERR("can't parse color '%s' (len: %d): %s", color_str.c_str(), + color_str.length(), reason); + delete[] reason; -parse_result parse_color_name(const std::string &name) { + return ERROR_COLOUR; +} + +std::optional parse_color_name(const std::string &name) { const rgb *value = color_name_hash::in_word_set(name.c_str(), name.length()); if (value != nullptr) { return Colour{value->red, value->green, value->blue}; } else { - return none(); + return no_colour(); } } -parse_result parse_hex_color(const std::string &color) { +std::optional parse_hex_color(const std::string &color) { const char *name = color.c_str(); size_t len = color.length(); // Skip a leading '#' if present. @@ -139,9 +110,7 @@ parse_result parse_hex_color(const std::string &color) { bool skip_alpha = (len == 3); for (size_t i = 0; i < len; i++) { int nib = hex_nibble_value(name[i]); - if (nib < 0) - return parse_result::error("invalid hex color: '%s' (%d)", - color.c_str(), color.length()); + if (nib < 0) return parse_error(color, "invalid hex value"); // Duplicate the nibble, so "#abc" -> 0xaa, 0xbb, 0xcc int val = (nib << 4) + nib; @@ -152,15 +121,13 @@ parse_result parse_hex_color(const std::string &color) { for (size_t i = 0; i + 1 < len; i += 2) { int nib1 = hex_nibble_value(name[i]); int nib2 = hex_nibble_value(name[i + 1]); - if (nib1 < 0 || nib2 < 0) - return parse_result::error("invalid hex color: '%s' (%d)", - color.c_str(), color.length()); + if (nib1 < 0 || nib2 < 0) return parse_error(color, "invalid hex value"); int val = (nib1 << 4) + nib2; argb[skip_alpha + i / 2] = val; } } else { - return none(); + return no_colour(); } return Colour(argb[1], argb[2], argb[3], argb[0]); @@ -169,17 +136,9 @@ parse_result parse_hex_color(const std::string &color) { Colour parse_color(const std::string &color) { std::optional result; -#define TRY_PARSER(name) \ - parse_result value_##name = name(color); \ - if (value_##name.has_colour()) { \ - return value_##name.get_colour().value(); \ - } else { \ - auto err = value_##name.get_error(); \ - if (err.has_value()) { \ - NORM_ERR(err.value().c_str()); \ - return ERROR_COLOUR; \ - } \ - } +#define TRY_PARSER(name) \ + std::optional value_##name = name(color); \ + if (value_##name.has_value()) { return value_##name.value(); } TRY_PARSER(parse_color_name) TRY_PARSER(parse_hex_color) From e64213073b15cbe8ce737901f121e3efb7732057 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tin=20=C5=A0vagelj?= Date: Sat, 25 May 2024 22:03:56 +0200 Subject: [PATCH 4/5] Reduce diff MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Make ctest print output on failure. Signed-off-by: Tin Švagelj --- .github/workflows/build-and-test-linux.yaml | 2 +- .github/workflows/build-and-test-macos.yaml | 2 +- src/colours.cc | 10 ++++------ 3 files changed, 6 insertions(+), 8 deletions(-) diff --git a/.github/workflows/build-and-test-linux.yaml b/.github/workflows/build-and-test-linux.yaml index b95caddc8..0e0ce99a2 100644 --- a/.github/workflows/build-and-test-linux.yaml +++ b/.github/workflows/build-and-test-linux.yaml @@ -169,4 +169,4 @@ jobs: run: cmake --build build - name: Test working-directory: build - run: ctest + run: ctest --output-on-failure diff --git a/.github/workflows/build-and-test-macos.yaml b/.github/workflows/build-and-test-macos.yaml index cfcf9a1b1..552b75b04 100644 --- a/.github/workflows/build-and-test-macos.yaml +++ b/.github/workflows/build-and-test-macos.yaml @@ -66,4 +66,4 @@ jobs: run: cmake --build build - name: Test working-directory: build - run: ctest + run: ctest --output-on-failure diff --git a/src/colours.cc b/src/colours.cc index 11e748798..16d8c8fd5 100644 --- a/src/colours.cc +++ b/src/colours.cc @@ -62,11 +62,9 @@ std::optional parse_error(const std::string &color_str, va_list args; va_start(args, format); size_t len = snprintf(nullptr, 0, format, args); - va_end(args); char *reason = new char[len + 1]; - va_start(args, format); - snprintf(reason, len, format, args); + snprintf(reason, len + 1, format, args); va_end(args); CRIT_ERR("can't parse color '%s' (len: %d): %s", color_str.c_str(), @@ -78,10 +76,10 @@ std::optional parse_error(const std::string &color_str, std::optional parse_color_name(const std::string &name) { const rgb *value = color_name_hash::in_word_set(name.c_str(), name.length()); - if (value != nullptr) { - return Colour{value->red, value->green, value->blue}; - } else { + if (value == nullptr) { return no_colour(); + } else { + return Colour{value->red, value->green, value->blue}; } } From 171b9619186e8396f26b250495ff6025fea88a18 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tin=20=C5=A0vagelj?= Date: Sat, 25 May 2024 22:11:51 +0200 Subject: [PATCH 5/5] Replace vararg function with template varargs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Tin Švagelj --- src/colours.cc | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/src/colours.cc b/src/colours.cc index 16d8c8fd5..72ff9dc09 100644 --- a/src/colours.cc +++ b/src/colours.cc @@ -31,7 +31,6 @@ #include "logging.h" -#include #include #include @@ -57,15 +56,13 @@ Colour Colour::from_argb32(uint32_t argb) { #endif /* BUILD_COLOUR_NAME_MAP */ std::optional inline no_colour() { return std::nullopt; } +template std::optional parse_error(const std::string &color_str, - const char *format...) { - va_list args; - va_start(args, format); - size_t len = snprintf(nullptr, 0, format, args); + const char *format, Args... args) { + size_t len = snprintf(nullptr, 0, format, args...); char *reason = new char[len + 1]; - snprintf(reason, len + 1, format, args); - va_end(args); + snprintf(reason, len + 1, format, args...); CRIT_ERR("can't parse color '%s' (len: %d): %s", color_str.c_str(), color_str.length(), reason); @@ -73,6 +70,12 @@ std::optional parse_error(const std::string &color_str, return ERROR_COLOUR; } +std::optional parse_error(const std::string &color_str, + const char *reason) { + CRIT_ERR("can't parse color '%s' (len: %d): %s", color_str.c_str(), + color_str.length(), reason); + return ERROR_COLOUR; +} std::optional parse_color_name(const std::string &name) { const rgb *value = color_name_hash::in_word_set(name.c_str(), name.length());