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

[Raman Tenneti via Phabricator via libc-commits]

rtenneti added a comment.

Addressed all the comments. PTAL.



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


================
Comment at: libc/src/time/time_zone_posix.cpp:190
+  PosixTransition posixTransition;
+  if (m_spec.starts_with(',')) {
+    m_spec.remove_prefix(1);
----------------
jeffbailey wrote:
> Should this be if!, and an early return?
It could start with either "," or "/" or both, thus we shouldn't return early. 


================
Comment at: libc/src/time/time_zone_posix.cpp:203
+    }
+    if (optionalPosixTransition.has_value())
+      posixTransition = optionalPosixTransition.value();
----------------
jeffbailey wrote:
> Should this have an else return Nullopt?  I don't think we'd process the /offset below if this part isn't set.
I don't know if the following TZ spec are valid or not. With this change, the following incomplete specs become invalid (we won't be defaulting). Hope it is okay.

+      "PST8PDT,",
+      "PST8PDT,M3",
+      "PST8PDT,M3.",
+      "PST8PDT,M3.2",
+      "PST8PDT,M3.2.",
+      "PST8PDT,M3.2.0,",

+      "PST8PDT,M3.2.0,M11",
+      "PST8PDT,M3.2.0,M11.",
+      "PST8PDT,M3.2.0,M11.1",
+      "PST8PDT,M3.2.0,M11.1.",




================
Comment at: libc/src/time/time_zone_posix.h:35
+public:
+  enum DateFormat : int { J, N, M };
+
----------------
tschuett wrote:
> enum class?
Defined it as enum so we don't need to do static cast.  We're relying on how it decays to an int


================
Comment at: libc/src/time/time_zone_posix.h:52
+
+    union {
+      NonLeapDay j;
----------------
tschuett wrote:
> Will this become the libc variant?
Added a TODO


================
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,
----------------
jeffbailey wrote:
> 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.
Deleted this file (as it is not needed)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130405