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

Remove unused util::detail::rand64bit method #2232

Merged
merged 1 commit into from Jul 1, 2022

Conversation

timwoj
Copy link
Contributor

@timwoj timwoj commented Jun 30, 2022

No description provided.

@timwoj
Copy link
Contributor Author

timwoj commented Jun 30, 2022

@0xxon I merged #2224 earlier not realizing that FreeBSD 12 builds are broken by it. Does this look reasonable as a fix?

@timwoj timwoj requested a review from 0xxon June 30, 2022 21:53
Copy link
Contributor

@bbannier bbannier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was wondering why this assertion was here in the first place, and believe it was probably added to ensure that both uint32_t halves of the uint64_t calculated here in below loop are distributed uniformly; if RAND_MAX is less than the maximum uint32_t this is not true anymore.

It looks like the value of RAND_MAX is also used implicitly here which then also causes prng and random_number to return values <=RAND_MAX.

Another issue is that differing RAND_MAX values would not make prng and its users platform-independent anymore like advertised.

TBH, I think this might need a better fix which requires more thinking. You might still want to merge this to unblock CI.

EDIT

Looks like FreeBSD changes the value of RAND_MAX to fix uniformity issues with rand. Arguing that system rand is often of poor quality usually anyway you might able to get away by just removing the static_assert here.

@0xxon
Copy link
Member

0xxon commented Jul 1, 2022

Looks reasonable for now - but yes, we should probably fix this differently :/

@0xxon
Copy link
Member

0xxon commented Jul 1, 2022

Thinking about it - this function is in zeek::detail and not used.

I move to just delete it completely.

@timwoj timwoj force-pushed the topic/timw/fix-RANDMAX-check branch from cf95220 to 8ed9543 Compare July 1, 2022 16:01
@timwoj
Copy link
Contributor Author

timwoj commented Jul 1, 2022

I move to just delete it completely.

Sounds good me. I pushed a commit that does that.

@timwoj timwoj changed the title Fix RAND_MAX check on FreeBSD 12 Remove unused util::rand64bit method Jul 1, 2022
@timwoj timwoj changed the title Remove unused util::rand64bit method Remove unused util::detail::rand64bit method Jul 1, 2022
@timwoj timwoj force-pushed the topic/timw/fix-RANDMAX-check branch from 8ed9543 to ecc1cb0 Compare July 1, 2022 17:26
@timwoj
Copy link
Contributor Author

timwoj commented Jul 1, 2022

@ckreibich confirmed this method isn't used by any packages in the package repo, so I'll go ahead and push this.

@timwoj timwoj force-pushed the topic/timw/fix-RANDMAX-check branch from ecc1cb0 to ef659b8 Compare July 1, 2022 21:10
@timwoj timwoj merged commit ba96843 into master Jul 1, 2022
@timwoj timwoj deleted the topic/timw/fix-RANDMAX-check branch July 11, 2022 18:01
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

3 participants