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

Fix for issue 943: improper parsing of hexadecimal and octal scalars #947

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

john-glover
Copy link

This is a fix for issue 943 that I reported. The root of the issue is that parsing sometimes surprisingly resulted in negative numbers and there was no way to specify some positive values with hexadecimal or octal scalars. The problem stems from trying to guess the width of the value because YAML does not provide a means of doing this (see issue 943 for details).

This changes the behavior for hexadecimal deserialization so I'm prepared for there to be some controversy here, but I think this is more in line with how a developer using the library might expect the deserialization to behave.

This also fixes the issue with octal parsing which was not functioning properly before.

@EdwardCooke
Copy link
Collaborator

I don't think an unisigned long is the most widely used number type. I would think that int is the most widely used as that is what the compiler seems to default as, example: var n = 0, n is an int. This would also end up being a breaking change due to changing the return types of the method being different. If you change this to doing an int, then long, then unsigned long I'd be happy to merge this in with the next breaking change build, but, I'm hoping that it will be a while before that happens again. The other alternative would be to change this up to be an option that can be set on the deserializerbuilder, something like a SmallestNumericDataType. Probably an option on the same method that enables the type conversion. Can't remember the name right off the top.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants