Make cctz::ParseDateTime() return NULL on failure

[higher-performance]

Prevents uninitialized data from being returned when the input is malformed (say, when the footer is \nPST8PDT,M3\n).

[devbww]

This and the next success = true case are redundant because p is already otherwise null.

So, I would suggest it is better handled without introducing extra state. To wit:

@@ -87,14 +87,17 @@ const char* ParseOffset(const char* p, int min_hour, int max_hour, int sign,
   return p;
 }
 
-// datetime = ( Jn | n | Mm.w.d ) [ / offset ]
+// datetime = , ( Jn | n | Mm.w.d ) [ / offset ]
 const char* ParseDateTime(const char* p, PosixTransition* res) {
-  if (p != nullptr && *p == ',') {
+  if (p != nullptr) {
+    if (*p != ',') return nullptr;
     if (*++p == 'M') {
       int month = 0;
-      if ((p = ParseInt(p + 1, 1, 12, &month)) != nullptr && *p == '.') {
+      if ((p = ParseInt(p + 1, 1, 12, &month)) != nullptr) {
+        if (*p != '.') return nullptr;
         int week = 0;
-        if ((p = ParseInt(p + 1, 1, 5, &week)) != nullptr && *p == '.') {
+        if ((p = ParseInt(p + 1, 1, 5, &week)) != nullptr) {
+          if (*p != '.') return nullptr;
           int weekday = 0;
           if ((p = ParseInt(p + 1, 0, 6, &weekday)) != nullptr) {
             res->date.fmt = PosixTransition::M;
@@ -117,10 +120,10 @@ const char* ParseDateTime(const char* p, PosixTransition* res) {
         res->date.n.day = static_cast<std::int_fast16_t>(day);
       }
     }
-  }
-  if (p != nullptr) {
-    res->time.offset = 2 * 60 * 60;  // default offset is 02:00:00
-    if (*p == '/') p = ParseOffset(p + 1, -167, 167, 1, &res->time.offset);
+    if (p != nullptr) {
+      res->time.offset = 2 * 60 * 60;  // default offset is 02:00:00
+      if (*p == '/') p = ParseOffset(p + 1, -167, 167, 1, &res->time.offset);
+    }
   }
   return p;
 }

[mkruskal-google]

Thanks Bradley, I'll admit that the added state did bug me I just didn't see any obvious alternative :)

[higher-performance]

Any chance we could keep the version in the PR? I'm having such a hard time wrapping my head around the alternative here. To be clear, the goal is to ensure that if we return non-NULL, then fmt is set correctly. I find that so easy to verify with the success variable, whereas I've been staring at this version for several minutes and I'm still not sure if I'm missing anything.

[devbww]

I think the easiest way to look at it is that on every error either nullptr is returned directly, or p is set to nullptr (to be returned later).

[higher-performance]

I'm not sure that helps much. What I find not at all obvious is how to prove that by the time res->time.offset is written to, res->date.fmt = ... has already occurred. With the success version, it's almost trivial to prove that. With this version, I have to study the code extremely carefully to convince myself that's the case -- I have to look at every single branch in the code to verify that p == nullptr forces an early return on every branch. That's not easy!

Notice ~50% of the branches in this function have side effects (if (*++p == 'M'), if ((p = ParseInt(p + 1, 0, 6, &weekday)) != nullptr), etc.) which obscure what the condition they're actually trying to test are (p != nullptr), and that makes it so much harder to understand the control flow.

Moreover, the code seems a lot more brittle that way: a seemingly innocuous change like changing } else { to } else if (*p != ...) { could easily lose the guarantee that res->time.offset is only written to when res->date.fmt has been set. It feels like it lays a trap that future maintainers just have to avoid falling into.

Is all of this really worth it just to get rid of a single bool?

[devbww]

What I find not at all obvious is how to prove that by the time res->time.offset is written to, res->date.fmt = ... has already occurred.

p != nullptr implies that res->date.fmt has been written to, and that is exactly the condition that is tested before writing to res-time.offset.

I have to look at every single branch in the code to verify that p == nullptr forces an early return on every branch.

Yes, I would say that you do need to look at every statement to verify behavior. However, I don't think you would need to verify that condition to conclude that a successful return now implies that *res has been fully assigned.

That said, I would be happy to get rid of the three early returns by converting them to if (*p != X) { p = nullptr; } else ... instead, but an early return nullptr is a feature of the other Parse*() functions in this file, so they didn't seem out of place.

Notice ~50% of the branches in this function have side effects (if (*++p == 'M'), if ((p = ParseInt(p + 1, 0, 6, &weekday)) != nullptr), etc.) which obscure what the condition they're actually trying to test

I don't subscribe to that line of thought. Indeed, I think that it clarifies that it is exactly the result of the side effect that is being tested.

Moreover, the code seems a lot more brittle that way: a seemingly innocuous change like changing } else { to } else if (*p != ...) { could easily lose the guarantee

Yes, any change could affect behavior, and deserves to analyzed. Seemingly innocuous and duplicating state are the traps; not having to follow control flow.

Is all of this really worth it just to get rid of a single bool?

To not introducing a redundant bool, I would say the answer is yes (which was my original comment).

Verification seems straightforward to me: any parse error results in a return nullptr (either by returning directly, or by clearing p), and returning a non-null result implies that *res has been correctly assigned. [And that last bit wasn't true in the *p != X cases before, which is why I absolutely appreciate your fix. Thanks!]

[devbww]

This looks unnecessary.

[devbww]

Thanks for introducing the test.

I don't expect you do this, but, if you have the inclination, this now affords the opportunity to add a more extensive test suite.