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 #538. Overflow check for unsigned values is removed. #539

Merged
merged 2 commits into from Feb 23, 2014

Conversation

Projects
None yet
2 participants
@NCrashed
Contributor

NCrashed commented Feb 19, 2014

Issue: #538

I've also added simple unittest to prevent such problem in future.

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Feb 20, 2014

Member

The idea of the to was indeed to prevent overflow during the conversion, but due to the way unsigned numbers need to be stored in BSON, I think a blend between how it was and how you changed it would be correct:

        else static if (is(T == uint)) return cast(uint)m_inputData.get!int();
        else static if (is(T : int)) return m_inputData.get!int().to!T;

So for smaller types short/ushort and the byte sized types it would still use to to prevent overflow, but for uint it would accept overflow on purpose. Anything that wouldn't be caught by that?

Member

s-ludwig commented Feb 20, 2014

The idea of the to was indeed to prevent overflow during the conversion, but due to the way unsigned numbers need to be stored in BSON, I think a blend between how it was and how you changed it would be correct:

        else static if (is(T == uint)) return cast(uint)m_inputData.get!int();
        else static if (is(T : int)) return m_inputData.get!int().to!T;

So for smaller types short/ushort and the byte sized types it would still use to to prevent overflow, but for uint it would accept overflow on purpose. Anything that wouldn't be caught by that?

@NCrashed

This comment has been minimized.

Show comment
Hide comment
@NCrashed

NCrashed Feb 20, 2014

Contributor

That's fine. The same logic should be applied for long/ulong:

else static if (is(T == ulong)) return cast(ulong)m_inputData.get!long();
else static if (is(T : long)) return m_inputData.get!long().to!T;
Contributor

NCrashed commented Feb 20, 2014

That's fine. The same logic should be applied for long/ulong:

else static if (is(T == ulong)) return cast(ulong)m_inputData.get!long();
else static if (is(T : long)) return m_inputData.get!long().to!T;
@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Feb 20, 2014

Member

It should be fine for long AFAICS because long and ulong are the only types that match (all smaller integers are handled by the int case).

Member

s-ludwig commented Feb 20, 2014

It should be fine for long AFAICS because long and ulong are the only types that match (all smaller integers are handled by the int case).

@NCrashed

This comment has been minimized.

Show comment
Hide comment
@NCrashed

NCrashed Feb 20, 2014

Contributor

I've catched the idea. The blended solution is commited.

Contributor

NCrashed commented Feb 20, 2014

I've catched the idea. The blended solution is commited.

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Feb 23, 2014

Member

Thank you!

Member

s-ludwig commented Feb 23, 2014

Thank you!

s-ludwig added a commit that referenced this pull request Feb 23, 2014

Merge pull request #539 from NCrashed/master
Fix #538. Overflow check for unsigned values is removed.

@s-ludwig s-ludwig merged commit 7506d63 into vibe-d:master Feb 23, 2014

1 check passed

default The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment