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

added random_device to intarray.cpp #5

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

malcolmk181
Copy link

No description provided.

Copy link
Owner

@seunomonije seunomonije left a comment

Choose a reason for hiding this comment

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

Good pr. see notes

random_array[i] = rand() % 2;
try
{
std::random_device rd;
Copy link
Owner

Choose a reason for hiding this comment

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

Move std::random device outside of the loop, calling it multiple times is redundant.

Copy link
Owner

@seunomonije seunomonije Feb 25, 2021

Choose a reason for hiding this comment

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

See here for an example of it being used. Otp we discussed and I read up some more on it.
std::random_device can fail

  • On construction, ie std::random_device [name], throwing an exception and exiting the whole program if left uncaught.
  • Or when requesting a random number, ie rd() % 2. Currently your code would catch this and use srand() in the same spot.

I think you have 2 options here. One is the easy way out and the other is more proper/more involved, but whichever you want to do is fine with me:

  • For our purposes it's fine if the whole program exits with an exception if std::random_device fails.
  • Possibly, a better way to construct this is to have 2 separate classes, one IntArraySrand and IntArrayRandDevice or something like that, and in a parent class IntArray attempt to build the array using IntArrayRandDevice and if that fails, build it using IntArraySrand. This way you could have std::random_device contained within the IntArrayRandDevice class and so if it fails when constructing the class you know you can't use it anymore.

Copy link
Collaborator

@nsnave nsnave Feb 26, 2021

Choose a reason for hiding this comment

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

The second option using the classes would also allow for easier reuse later; notably, being able to combine the generators in different ways for different purposes that appeal better to the different rngs' strengths.

{
std::random_device rd;
random_array[i] = rd() % 2;
} catch(...)
Copy link
Owner

@seunomonije seunomonije Feb 25, 2021

Choose a reason for hiding this comment

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

Try catch rather than actually checking for errors? EDIT: I actually like the try catch. See above.

@seunomonije seunomonije self-requested a review February 25, 2021 23:29
random_array[i] = rand() % 2;
try
{
std::random_device rd;
Copy link
Owner

@seunomonije seunomonije Feb 25, 2021

Choose a reason for hiding this comment

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

See here for an example of it being used. Otp we discussed and I read up some more on it.
std::random_device can fail

  • On construction, ie std::random_device [name], throwing an exception and exiting the whole program if left uncaught.
  • Or when requesting a random number, ie rd() % 2. Currently your code would catch this and use srand() in the same spot.

I think you have 2 options here. One is the easy way out and the other is more proper/more involved, but whichever you want to do is fine with me:

  • For our purposes it's fine if the whole program exits with an exception if std::random_device fails.
  • Possibly, a better way to construct this is to have 2 separate classes, one IntArraySrand and IntArrayRandDevice or something like that, and in a parent class IntArray attempt to build the array using IntArrayRandDevice and if that fails, build it using IntArraySrand. This way you could have std::random_device contained within the IntArrayRandDevice class and so if it fails when constructing the class you know you can't use it anymore.

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.

3 participants