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

(Issue #277) Make CXXGraph MSVC-Compatible #280

Merged
merged 5 commits into from
May 8, 2023
Merged

Conversation

nrkramer
Copy link
Collaborator

@nrkramer nrkramer commented May 7, 2023

In this PR, we're switching out calls to srand and rand_r (in which rand_r is a Unix-only function included in <stdlib.h>), with usages of std::random constructs.

These constructs are initialized in a thread-safe manner using TLS:

  thread_local static std::default_random_engine rand;
  thread_local static std::uniform_int_distribution distribution(0, RAND_MAX);

  rand.seed(some_seed);
  distribution(rand); // Computes the next random number in the sequence.

The reason we use TLS is to pay the initialization cost only once per-thread. This ensures thread-safety of the PRNG sequence, as well as providing a decent tradeoff between performance and memory.

  • A reference to a related issue in your repository.
  • A description of the changes proposed in the pull request.
    • Switch usages of srand and rand_r to the more cross-platform <random> implementations.
    • Remove 2 test asserts that relied on comparisons of platform-dependent hashing implementations.
  • @mentions of the person or team responsible for reviewing proposed changes. ( optional )

@github-actions github-actions bot added core something about core repo something about repo test Something about test labels May 7, 2023
@ghost
Copy link

ghost commented May 7, 2023

👇 Click on the image for a new way to code review

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map legend

README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
@ZigRazor ZigRazor linked an issue May 8, 2023 that may be closed by this pull request
@ZigRazor ZigRazor self-assigned this May 8, 2023
@ZigRazor ZigRazor self-requested a review May 8, 2023 06:16
@ZigRazor ZigRazor merged commit 04c6cb7 into ZigRazor:master May 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core something about core repo something about repo test Something about test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make CXXGraph MSVC-Compatible
2 participants