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

Jeff Bailey via Phabricator via libc-commits libc-commits at lists.llvm.org
Tue Jul 26 10:59:52 PDT 2022

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
+    __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,

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.

  rG LLVM Github Monorepo



More information about the libc-commits mailing list