Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[libc++] Improve diagnostic when failing to parse the tzdb #122125

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

ldionne
Copy link
Member

@ldionne ldionne commented Jan 8, 2025

Providing the character that we failed on is helpful for figuring out what's going wrong in the tztb.

Providing the character that we failed on is helpful for figuring
out what's going wrong in the tztb.
@ldionne ldionne requested a review from a team as a code owner January 8, 2025 15:22
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Jan 8, 2025
@llvmbot
Copy link
Member

llvmbot commented Jan 8, 2025

@llvm/pr-subscribers-libcxx

Author: Louis Dionne (ldionne)

Changes

Providing the character that we failed on is helpful for figuring out what's going wrong in the tztb.


Full diff: https://github.com/llvm/llvm-project/pull/122125.diff

3 Files Affected:

  • (modified) libcxx/src/experimental/tzdb.cpp (+15-5)
  • (modified) libcxx/test/libcxx/time/time.zone/time.zone.db/rules.pass.cpp (+10-5)
  • (modified) libcxx/test/libcxx/time/time.zone/time.zone.db/version.pass.cpp (+3-2)
diff --git a/libcxx/src/experimental/tzdb.cpp b/libcxx/src/experimental/tzdb.cpp
index 638d45f69e033e..10f7001d74806b 100644
--- a/libcxx/src/experimental/tzdb.cpp
+++ b/libcxx/src/experimental/tzdb.cpp
@@ -8,6 +8,7 @@
 
 // For information see https://libcxx.llvm.org/DesignDocs/TimeZone.html
 
+#include <__assert>
 #include <algorithm>
 #include <cctype>
 #include <chrono>
@@ -97,14 +98,23 @@ static void __skip(istream& __input, string_view __suffix) {
 }
 
 static void __matches(istream& __input, char __expected) {
-  if (std::tolower(__input.get()) != __expected)
-    std::__throw_runtime_error((string("corrupt tzdb: expected character '") + __expected + '\'').c_str());
+  _LIBCPP_ASSERT_INTERNAL(std::islower(__expected), "lowercase characters only here!");
+  char __c = __input.get();
+  if (std::tolower(__c) != __expected)
+    std::__throw_runtime_error(
+        (string("corrupt tzdb: expected character '") + __expected + "', got '" + __c + "' instead").c_str());
 }
 
 static void __matches(istream& __input, string_view __expected) {
-  for (auto __c : __expected)
-    if (std::tolower(__input.get()) != __c)
-      std::__throw_runtime_error((string("corrupt tzdb: expected string '") + string(__expected) + '\'').c_str());
+  for (auto __c : __expected) {
+    _LIBCPP_ASSERT_INTERNAL(std::islower(__c), "lowercase strings only here!");
+    char __actual = __input.get();
+    if (std::tolower(__actual) != __c)
+      std::__throw_runtime_error(
+          (string("corrupt tzdb: expected character '") + __c + "' from string '" + string(__expected) + "', got '" +
+           __actual + "' instead")
+              .c_str());
+  }
 }
 
 [[nodiscard]] static string __parse_string(istream& __input) {
diff --git a/libcxx/test/libcxx/time/time.zone/time.zone.db/rules.pass.cpp b/libcxx/test/libcxx/time/time.zone/time.zone.db/rules.pass.cpp
index 7d9759320c535b..237a206b3a95bd 100644
--- a/libcxx/test/libcxx/time/time.zone/time.zone.db/rules.pass.cpp
+++ b/libcxx/test/libcxx/time/time.zone/time.zone.db/rules.pass.cpp
@@ -20,9 +20,10 @@
 // ADDITIONAL_COMPILE_FLAGS: -I %{libcxx-dir}/src/experimental/include
 
 #include <chrono>
+#include <cstdio>
 #include <fstream>
-#include <string>
 #include <string_view>
+#include <string>
 #include <variant>
 
 #include "assert_macros.h"
@@ -96,7 +97,7 @@ static void test_invalid() {
   test_exception("R r 0 mix", "corrupt tzdb: expected whitespace");
   test_exception("R r 0 1", "corrupt tzdb: expected whitespace");
 
-  test_exception("R r 0 1 X", "corrupt tzdb: expected character '-'");
+  test_exception("R r 0 1 X", "corrupt tzdb: expected character '-', got 'X' instead");
 
   test_exception("R r 0 1 -", "corrupt tzdb: expected whitespace");
 
@@ -106,13 +107,17 @@ static void test_invalid() {
 
   test_exception("R r 0 1 - Ja +", "corrupt tzdb weekday: invalid name");
   test_exception("R r 0 1 - Ja 32", "corrupt tzdb day: value too large");
-  test_exception("R r 0 1 - Ja l", "corrupt tzdb: expected string 'last'");
+  test_exception(
+      "R r 0 1 - Ja l",
+      std::string{"corrupt tzdb: expected character 'a' from string 'last', got '"} + (char)EOF + "' instead");
   test_exception("R r 0 1 - Ja last", "corrupt tzdb weekday: invalid name");
   test_exception("R r 0 1 - Ja lastS", "corrupt tzdb weekday: invalid name");
   test_exception("R r 0 1 - Ja S", "corrupt tzdb weekday: invalid name");
   test_exception("R r 0 1 - Ja Su", "corrupt tzdb on: expected '>=' or '<='");
-  test_exception("R r 0 1 - Ja Su>", "corrupt tzdb: expected character '='");
-  test_exception("R r 0 1 - Ja Su<", "corrupt tzdb: expected character '='");
+  test_exception(
+      "R r 0 1 - Ja Su>", std::string{"corrupt tzdb: expected character '=', got '"} + (char)EOF + "' instead");
+  test_exception(
+      "R r 0 1 - Ja Su<", std::string{"corrupt tzdb: expected character '=', got '"} + (char)EOF + "' instead");
   test_exception("R r 0 1 - Ja Su>=+", "corrupt tzdb: expected a non-zero digit");
   test_exception("R r 0 1 - Ja Su>=0", "corrupt tzdb: expected a non-zero digit");
   test_exception("R r 0 1 - Ja Su>=32", "corrupt tzdb day: value too large");
diff --git a/libcxx/test/libcxx/time/time.zone/time.zone.db/version.pass.cpp b/libcxx/test/libcxx/time/time.zone/time.zone.db/version.pass.cpp
index b4f32a1b6fd785..ca3a890f1fa548 100644
--- a/libcxx/test/libcxx/time/time.zone/time.zone.db/version.pass.cpp
+++ b/libcxx/test/libcxx/time/time.zone/time.zone.db/version.pass.cpp
@@ -18,9 +18,10 @@
 // This is not part of the public tzdb interface.
 
 #include <chrono>
+#include <cstdio>
 #include <fstream>
-#include <string>
 #include <string_view>
+#include <string>
 
 #include "assert_macros.h"
 #include "concat_macros.h"
@@ -60,7 +61,7 @@ static void test_exception(std::string_view input, [[maybe_unused]] std::string_
 }
 
 int main(int, const char**) {
-  test_exception("", "corrupt tzdb: expected character '#'");
+  test_exception("", std::string{"corrupt tzdb: expected character '#', got '"} + (char)EOF + "' instead");
   test_exception("#version", "corrupt tzdb: expected whitespace");
   test("#version     \t                      ABCD", "ABCD");
   test("#Version     \t                      ABCD", "ABCD");

@ldionne ldionne added the pending-ci Merging the PR is only pending completion of CI label Jan 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. pending-ci Merging the PR is only pending completion of CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants