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

Check first and then do increment #1636

Closed
wants to merge 1 commit into from

Conversation

SunFulong
Copy link
Contributor

Wrapping around at the boundary should do check first.

@oberstet
Copy link
Contributor

oberstet commented Apr 13, 2024

This is wrong. It won't create an ID 9007199254740992. It is also unnecessary, the code as it stands is correct.


the bug I mentioned in the other thread is on this line:

return random.randint(0, 9007199254740992)

The 0 must be 1 ...

@oberstet oberstet closed this Apr 13, 2024
@SunFulong
Copy link
Contributor Author

It won't create an ID 9007199254740992.

Although it won't created, but it can be reached at previous round.

@oberstet
Copy link
Contributor

Although it won't created, but it can be reached at previous round.

yes, you are right actually, it would spit out 9007199254740992. however, I stand by my other comment: it is unnecessary (IOW: the PR code does not change the functional behavior in any way. and non-functional behavior .. performance .. I am not convinced it would be an improvement ... or even necessary anyways.

if you file a PR that changes the 0 to 1 to fix the bug I mentioned above, that would be highly welcome! I'd merged it right away ...

@SunFulong SunFulong deleted the patch-1 branch April 13, 2024 06:43
@SunFulong
Copy link
Contributor Author

if you file a PR that changes the 0 to 1 to fix the bug

See #1637

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.

None yet

2 participants