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
Apply some C++ love to string handling in file and config paths. #8362
Conversation
e85446a
to
a0a674e
Compare
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.
Changed code looks fine, afaict.
Have you got a way to find all the "new" unnecessary .c_str()
calls? passing strings between std::string and char* and back again could add up performance-wise quite quickly...
/* std::unique_ptr assumes new/delete unless a custom deleter is supplied. | ||
* As we don't want to have to carry that deleter all over the place, use | ||
* new directly to allocate the memory instead of malloc. */ | ||
std::unique_ptr<char> mem(static_cast<char *>(::operator new(len + 1))); |
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.
Why so complex?
std::unique_ptr<char> mem(static_cast<char *>(::operator new(len + 1))); | |
std::unique_ptr<char[]> mem(new char[len + 1]); |
No?
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.
new[] has to be freed with delete[], otherwise it's undefined behaviour. As we coerce the memory to a std::unique_ptr<LanguagePack>
pointer later, it will call normal delete.
LanguagePack *lang_pack = (LanguagePack *)ReadFileToMem(lang->file, &len, 1U << 20); | ||
if (lang_pack == nullptr) return false; | ||
size_t len = 0; | ||
std::unique_ptr<LanguagePack> lang_pack(reinterpret_cast<LanguagePack *>(ReadFileToMem(lang->file, len, 1U << 20).release())); |
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.
Yikes.
Perhaps templating ReadFileToMem
might be worth doing?
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.
Looks ugly in the header file, but yeah, might indeed be better.
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.
Doesn't really solve the memory allocation problem tough, LanguagePack
is of undefined size and we still need to obtain a non-array block of memory using new
.
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.
Looking at it again, this is the only use of ReadFileToMem. Perhaps just specialise the function as appropriate?
|
fe1fae3
to
68c82d0
Compare
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.
I see no obvious issues in the code, other than the non-blocking stuff mentioned previously
Remove most char* usage in FIO code.