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

[Siva Chandra via Phabricator via libc-commits]

sivachandra added a comment.

Sorry for not getting to this earlier. I have left a few high level comments. I will do another pass once they are addressed.



================
Comment at: libc/src/time/time_zone_posix.cpp:3
+
+#include "src/ctype/isdigit.h"
+#include "src/stdlib/strtol.h"
----------------
Prefer using internal ctype utilities: https://github.com/llvm/llvm-project/blob/main/libc/src/__support/ctype_utils.h


================
Comment at: libc/src/time/time_zone_posix.cpp:25
+// is less than min or greater than max.
+cpp::optional<int> PosixTimeZone::ParseInt(int min, int max) {
+  if (spec.empty())
----------------
I know that it is not considered idiomatic to pass a `string_view` object by reference in general. But, in this case, it seems like a good thing to pass the `spec` by non-const-reference to these helper methods? It will avoid storing `spec` as a state variable.


================
Comment at: libc/src/time/time_zone_posix.cpp:29
+
+  char *pEnd;
+  int value = strtol(spec.data(), &pEnd, 10); // NOLINT(runtime/deprecated_fn)
----------------
LLVM libc style convention is to use `snake_case` for variable names.


================
Comment at: libc/src/time/time_zone_posix.cpp:30
+  char *pEnd;
+  int value = strtol(spec.data(), &pEnd, 10); // NOLINT(runtime/deprecated_fn)
+  if (value < min || value > max)
----------------
`strtol` and the internel string to integer converter [1] have some gotchas: For example, they allow prefix spaces like `      123`. Is that OK or should we error out for such strings? Also, `strtol` can fail or return 0 silently, which probably is OK/irrelevant here?

[1] - https://github.com/llvm/llvm-project/blob/main/libc/src/__support/str_to_integer.h


================
Comment at: libc/src/time/time_zone_posix.cpp:61
+  for (const auto &p : spec) {
+    if (strchr("-+,", p))
+      break;
----------------
Can we use `string_view::find_first_of`?


================
Comment at: libc/src/time/time_zone_posix.cpp:176
+  posixTransition.date.fmt = PosixTransition::DateFormat::M;
+  posixTransition.date.m.month = static_cast<int8_t>(month);
+  posixTransition.date.m.week = static_cast<int8_t>(week);
----------------
You can reduce the verbosity of the `static_cast` with just `uint8_t(month)`.


================
Comment at: libc/src/time/time_zone_posix.cpp:283
+// time zone and UTC. In ISO 8601, the particular zone offset can be indicated
+// in a date or time value. The zone offset can be Z for UTC or it can be a
+// value "+" or "-" from UTC.
----------------
Is `Z` permitted in the string for the `TZ` env var? Also, is `ParseOffset` handling it?


================
Comment at: libc/src/time/time_zone_posix.h:12
+
+#include <stdint.h> // int8_t, int16_t and int32_t
+
----------------
Duplicate?


================
Comment at: libc/src/time/time_zone_posix.h:85
+  struct Date {
+    struct NonLeapDay {
+      int16_t day; // day of non-leap year [1:365]
----------------
Why are these a `struct` instead of a `typedef` like this:

```
typedef uint16_t NonLeapDay;
```


================
Comment at: libc/src/time/time_zone_posix.h:142
+// daylight time occurs first in any particular year.
+class PosixTimeZone {
+public:
----------------
Nit: Do we really want the verbosity of `PosixTimeZone`? Is `TZInfo` not good enough?


================
Comment at: libc/src/time/time_zone_posix.h:166
+  static cpp::optional<PosixTimeZone>
+  ParsePosixSpec(const cpp::string_view spec);
+
----------------
LLVM libc's style is to use `snake_case` for function and method names. So, this and the private methods below should be fixed. Also, would the name `parse_tz_string` be more appropriate?


================
Comment at: libc/src/time/time_zone_posix.h:168
+
+  cpp::string_view spec;
+
----------------
Should `spec` be part of the state, especially when it really denotes nothing after parsing? See below for related comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130405