-
Notifications
You must be signed in to change notification settings - Fork 38.7k
change "char pch[200000]" to "new char[200000]" #4236
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
Conversation
src/util.cpp
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sizeof(pch) will be 4 or 8 here. You should use the correct amount (probably best to define a constant for the 200000).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're going to use dynamic memory, please use a properly managed way
(e.g. use a vector).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Switching to dynamic memory makes sense, 200KB is too much to allocate on the stack. It could exhaust the per-thread stack on some platforms.
Using a vector sounds good to me.
|
Indeed. An RAII vector is superior to something you must remember to manually delete[]. |
|
@arowser are you going to update this to use a vector instead? |
|
@laanwj Yes, I'll update it later, sorry for late. I'm have more than ten years experience in c lang, but I'm a c++ new guy, so I'm studying how change char * to vector in c++ and make sure the chang is ok before I push it again, thanks (: |
src/util.cpp
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use
std::vector<unsigned char>
instead.
|
Looks OK, but I'm a bit wary to merge as it affects entropy collection for randomness. I'd like someone with windows to verify that this does the right thing. |
|
Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4236_ea7875e8c3da92b0cbddb9c794843cfcf6c0a80c/ for binaries and test log. |
|
Closing in favor of #4392 |
Change the big size local var "char pch[200000]" to "new char[200000]" to avoid the possibly stack overflow.