From 2fe7f19026088e7cd0d1510f44fbcd4aef925c21 Mon Sep 17 00:00:00 2001 From: dan sinclair Date: Tue, 7 Mar 2023 18:20:23 +0000 Subject: [PATCH] Use `absl::from_chars` instead of `strtod` This CL switches Tint to use `absl::from_chars` instead of `strtod` in order to have locale independent parsing of floats. Bug: tint:1686 Change-Id: Icb3d9a40928a1cb154b32629f7e39173d80e4563 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/122780 Reviewed-by: David Neto Kokoro: Kokoro Commit-Queue: Dan Sinclair --- src/tint/BUILD.gn | 13 +++++++++ src/tint/CMakeLists.txt | 4 +-- src/tint/reader/wgsl/lexer.cc | 47 ++++++++++++++++++++++-------- src/tint/reader/wgsl/lexer_test.cc | 2 +- 4 files changed, 51 insertions(+), 15 deletions(-) diff --git a/src/tint/BUILD.gn b/src/tint/BUILD.gn index 89eb6eef99..6479684a8b 100644 --- a/src/tint/BUILD.gn +++ b/src/tint/BUILD.gn @@ -14,6 +14,7 @@ import("//build_overrides/build.gni") +import("../../scripts/dawn_overrides_with_defaults.gni") import("../../tint_overrides_with_defaults.gni") if (tint_build_unittests) { @@ -92,6 +93,17 @@ config("tint_config") { } } +group("abseil") { + # When build_with_chromium=true we need to include "//third_party/abseil-cpp:absl" while + # it's beneficial to be more specific with standalone Dawn, especially when it comes to + # including it as a dependency in other projects (such as Skia). + if (build_with_chromium) { + public_deps = [ "$dawn_abseil_dir:absl" ] + } else { + public_deps = [ "${dawn_root}/third_party/gn/abseil-cpp:strings" ] + } +} + ############################################################################### # Helper library for IO operations # Only to be used by tests and sample executable @@ -953,6 +965,7 @@ libtint_source_set("libtint_wgsl_reader_src") { ] deps = [ + ":abseil", ":libtint_ast_src", ":libtint_base_src", ":libtint_builtins_src", diff --git a/src/tint/CMakeLists.txt b/src/tint/CMakeLists.txt index 1d31896250..6a8f503fbc 100644 --- a/src/tint/CMakeLists.txt +++ b/src/tint/CMakeLists.txt @@ -760,7 +760,7 @@ target_link_libraries(tint_val tint_utils_io) ## Tint library add_library(libtint ${TINT_LIB_SRCS}) tint_default_compile_options(libtint) -target_link_libraries(libtint tint_diagnostic_utils) +target_link_libraries(libtint tint_diagnostic_utils absl_strings) if (${TINT_SYMBOL_STORE_DEBUG_NAME}) target_compile_definitions(libtint PUBLIC "TINT_SYMBOL_STORE_DEBUG_NAME=1") endif() @@ -770,7 +770,7 @@ if (${TINT_BUILD_FUZZERS}) # Tint library with fuzzer instrumentation add_library(libtint-fuzz ${TINT_LIB_SRCS}) tint_default_compile_options(libtint-fuzz) - target_link_libraries(libtint-fuzz tint_diagnostic_utils) + target_link_libraries(libtint-fuzz tint_diagnostic_utils absl_strings) if (${COMPILER_IS_LIKE_GNU}) target_compile_options(libtint-fuzz PRIVATE -fvisibility=hidden) endif() diff --git a/src/tint/reader/wgsl/lexer.cc b/src/tint/reader/wgsl/lexer.cc index 28fb135672..91c2993332 100644 --- a/src/tint/reader/wgsl/lexer.cc +++ b/src/tint/reader/wgsl/lexer.cc @@ -25,6 +25,7 @@ #include #include +#include "absl/strings/charconv.h" #include "src/tint/debug.h" #include "src/tint/number.h" #include "src/tint/text/unicode.h" @@ -352,9 +353,11 @@ Token Lexer::try_float() { // Parse the exponent if one exists bool has_exponent = false; + bool negative_exponent = false; if (end < length() && (matches(end, 'e') || matches(end, 'E'))) { end++; if (end < length() && (matches(end, '+') || matches(end, '-'))) { + negative_exponent = matches(end, '-'); end++; } @@ -374,10 +377,8 @@ Token Lexer::try_float() { bool has_f_suffix = false; bool has_h_suffix = false; if (end < length() && matches(end, 'f')) { - end++; has_f_suffix = true; } else if (end < length() && matches(end, 'h')) { - end++; has_h_suffix = true; } @@ -386,29 +387,51 @@ Token Lexer::try_float() { return {}; } - advance(end - start); - end_source(source); + // Note, the `at` method will return a static `0` if the provided position is >= length. We + // actually need the end pointer to point to the correct memory location to use `from_chars`. + // So, handle the case where we point past the length specially. + auto* end_ptr = &at(end); + if (end >= length()) { + end_ptr = &at(length() - 1) + 1; + } - double value = std::strtod(&at(start), nullptr); + double value = 0; + auto ret = absl::from_chars(&at(start), end_ptr, value); + bool overflow = ret.ec != std::errc(); + + // The provided value did not fit in a double and has a negative exponent, so treat it as a 0. + if (negative_exponent && ret.ec == std::errc::result_out_of_range) { + overflow = false; + value = 0.0; + } + + TINT_ASSERT(Reader, end_ptr == ret.ptr); + advance(end - start); if (has_f_suffix) { - if (auto f = CheckedConvert(AFloat(value))) { + auto f = CheckedConvert(AFloat(value)); + if (!overflow && f) { + advance(1); + end_source(source); return {Token::Type::kFloatLiteral_F, source, static_cast(f.Get())}; - } else { - return {Token::Type::kError, source, "value cannot be represented as 'f32'"}; } + return {Token::Type::kError, source, "value cannot be represented as 'f32'"}; } if (has_h_suffix) { - if (auto f = CheckedConvert(AFloat(value))) { + auto f = CheckedConvert(AFloat(value)); + if (!overflow && f) { + advance(1); + end_source(source); return {Token::Type::kFloatLiteral_H, source, static_cast(f.Get())}; - } else { - return {Token::Type::kError, source, "value cannot be represented as 'f16'"}; } + return {Token::Type::kError, source, "value cannot be represented as 'f16'"}; } + end_source(source); + TINT_BEGIN_DISABLE_WARNING(FLOAT_EQUAL); - if (value == HUGE_VAL || -value == HUGE_VAL) { + if (overflow || value == HUGE_VAL || -value == HUGE_VAL) { return {Token::Type::kError, source, "value cannot be represented as 'abstract-float'"}; } else { return {Token::Type::kFloatLiteral, source, value}; diff --git a/src/tint/reader/wgsl/lexer_test.cc b/src/tint/reader/wgsl/lexer_test.cc index 95b43a7da0..a2442731e6 100644 --- a/src/tint/reader/wgsl/lexer_test.cc +++ b/src/tint/reader/wgsl/lexer_test.cc @@ -397,7 +397,7 @@ TEST_P(FloatTest, Parse) { Lexer l(&file); auto list = l.Lex(); - ASSERT_EQ(2u, list.size()); + ASSERT_EQ(2u, list.size()) << "Got: " << list[0].to_str(); { auto& t = list[0];