Skip to content

Fixed bug #74300 #2429

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

Closed
wants to merge 1 commit into from
Closed

Conversation

xKerman
Copy link
Contributor

@xKerman xKerman commented Mar 23, 2017

@krakjoe
Copy link
Member

krakjoe commented Mar 23, 2017

var_unserializer.c is generated code, we do not change it directly.

@xKerman
Copy link
Contributor Author

xKerman commented Mar 23, 2017

@krakjoe Thanks, so is it better excluding var_unserializer.c from this pull request?

@xKerman xKerman force-pushed the fix/unserialize-nvexp-part branch from 0d5dcb6 to 8c0c92e Compare March 23, 2017 16:05
@xKerman
Copy link
Contributor Author

xKerman commented Mar 23, 2017

Updated pull request not to include var_unserializer.c for diff.

@krakjoe
Copy link
Member

krakjoe commented Mar 23, 2017

Oh sorry, I missed the change to generator input and thought only the generated parser was changed ... this should include the change to the generated parser, and the generator input ...

@xKerman
Copy link
Contributor Author

xKerman commented Mar 23, 2017

Oh, I see, I will include var_unserializer.c for diff.

@xKerman xKerman force-pushed the fix/unserialize-nvexp-part branch from 8c0c92e to 0d5dcb6 Compare March 23, 2017 16:11
@xKerman
Copy link
Contributor Author

xKerman commented Mar 23, 2017

Updated (include var_unserializer.c for diff)

@krakjoe
Copy link
Member

krakjoe commented Mar 23, 2017

@smalyshev can I get a thumbs up for this please ?

@nikic
Copy link
Member

nikic commented Mar 23, 2017

Merged via 6a1d4cd into master. I didn't apply this change to stable branches, because it is technically BC breaking and there is no real problem with continuing to accept this input. (And yes, we had people complain about changes like this before.)

@nikic nikic closed this Mar 23, 2017
@nikic
Copy link
Member

nikic commented Mar 23, 2017

Issued mentioned in the previous comment fixed with 8522e28.

@smalyshev
Copy link
Contributor

@krakjoe Looks fine to me. Agree with @nikic that it is not really required for stable branches, but no reason to keep accepting this further.

@xKerman
Copy link
Contributor Author

xKerman commented Mar 23, 2017

Thanks!

@xKerman xKerman deleted the fix/unserialize-nvexp-part branch March 23, 2017 22:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants