[libc-commits] [PATCH] D130405: Implement TZ Variable Parsing Function.

[Jeff Bailey via Phabricator via libc-commits]

jeffbailey added a comment.

Took a quick review while I had a break this morning.



================
Comment at: libc/src/time/time_zone_posix.cpp:18
+cpp::Optional<int> PosixTimeZone::ParseInt(int min, int max) {
+  if (m_spec.size() == 0)
+    return cpp::Nullopt;
----------------
(Here and below)


================
Comment at: libc/src/time/time_zone_posix.cpp:68
+cpp::Optional<int32_t> PosixTimeZone::ParseOffset(int min_hour, int max_hour,
+                                                  int sign) {
+  if (m_spec.size() == 0)
----------------
I keep wondering if this should be bool instead of int.  I think it's always plus one or negative one, never zero or another value.


================
Comment at: libc/src/time/time_zone_posix.cpp:85
+  cpp::Optional<int> result = ParseInt(min_hour, max_hour);
+  if (!result.has_value())
+    return cpp::Nullopt;
----------------
The various `.has_value()` are redundant as optional supports `operator bool` for these. (Here and below)


================
Comment at: libc/src/time/time_zone_posix.cpp:120
+  // Parse month.
+  cpp::Optional<int> result = ParseInt(1, 12);
+  if (!result.has_value())
----------------
Could ParseInt be a template and return int8_t, unit16_t, etc?  That way the static_casts below wouldn't be needed.  


================
Comment at: libc/src/time/time_zone_posix.cpp:237
+bool PosixTimeZone::UpdateStdOffset() {
+  const cpp::Optional<int32_t> offset = ParseOffset(0, 24, -1);
+  if (!offset.has_value())
----------------
We should have a comment saying why this is negative here.  Especially since if what it parses after this has a negative, it will invert this to a positive.

(Also in UpdateDstOffset)


================
Comment at: libc/test/src/time/TZHelper.h:20
+static inline __llvm_libc::time_zone_posix::PosixTimeZone
+initialize_posix_time_zone_data(
+    __llvm_libc::cpp::StringView spec, __llvm_libc::cpp::StringView std_abbr,
----------------
Instead of this could the callers just use a designated initializer directly?

```PosixTimeZone res{
  .m_std_abbr = std_abbr,
  .m_std_offset = std_offset,
  (etc...)
```

That way this function and the others here wouldn't be needed and it would be clearer when reading it that the arguments aren't accidentally in the wrong order.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130405/new/

https://reviews.llvm.org/D130405