Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fixes bug #65657, ISO-8601 compliant periods being rejected by parser #2598

Closed
wants to merge 6 commits into from

Conversation

@jcomeauictx
Copy link

jcomeauictx commented Jun 27, 2017

a lot of the parser code is pretty dodgy, and doesn't appear to be using re2c to good advantage, but I focused on fixing the immediate problem with minimal coding changes. I added a test for the bug, verified that it failed before I started coding, and now it passes on both x86_64 and ppc64le virtual hosts.

there is no Makefile rule for creating ext/date/lib/parse_iso_intervals.c from its corresponding .re file, so I ran re2c --no-generation-date -b -o ext/date/lib/parse_iso_intervals.c ext/date/lib/parse_iso_intervals.re. perhaps a rule should be added to Makefile.frag, but I didn't want to dig into how that whole thing works and would rather not step on toes. DRY would suggest having only the .re files in the repository if the .c files are generated from them.

@jcomeauictx

This comment has been minimized.

Copy link
Author

jcomeauictx commented Jun 28, 2017

I see the memory leak in the 2nd travis run. tried to get rid of it by defining char *ZULU_TIME = "Z"; globally and setting t->tz_abbr = ZULU_TIME; but then I get zend_mm_heap corrupted. I'm probably out of my league here. I was never too brilliant about keeping track of pointers and mallocs.

@KalleZ

This comment has been minimized.

Copy link
Member

KalleZ commented Jun 28, 2017

Hi

The fix should go to derickr/timelib, where the timelib is maintained, and only contain the test once committed and synced.

cc @derickr

@krakjoe

This comment has been minimized.

Copy link
Member

krakjoe commented Jun 28, 2017

As above, please open fresh with test only when upstream has changes ...

@krakjoe krakjoe closed this Jun 28, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
You can’t perform that action at this time.