Hi, > Since I had this on my (ever-growing) TODO I re-prioritized today and took a > look at it since I think it's something we should support. > > Nothing really sticks out and I was unable to poke any holes so I don't have > too much more to offer than a LGTM. > > + while (len > 0) > + { > + const datetkn *tp = datebsearch(lowtoken, zoneabbrevtbl->abbrevs, > + zoneabbrevtbl->numabbrevs); > > My immediate reaction was that we should stop at prefix lengths of two since I > could only think of abbreviations of two or more. Googling and reading found > that there are indeed one-letter timezones (Alpha, Bravo etc..). Not sure if > it's worth mentioning that in the comment to help other readers who aren't > neck > deep in timezones? > > + /* FALL THRU */ > > Tiny nitpick, it looks a bit curious that we spell it FALL THRU here and "fall > through" a few cases up in the same switch. While we are quite inconsistent > across the tree, consistency within a file is preferrable (regardless of > which).
I reviewed the patch and tested it on MacOS and generally concur with stated above. The only nitpick I have is the apparent lack of negative tests for to_timestamp(), e.g. when the string doesn't match the specified format. -- Best regards, Aleksander Alekseev