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

Apply some C++ love to string handling in file and config paths. #8362

Merged
merged 8 commits into from Dec 27, 2020

Conversation

michicc
Copy link
Member

@michicc michicc commented Dec 6, 2020

Remove most char* usage in FIO code.

@michicc michicc force-pushed the pr/string branch 2 times, most recently from e85446a to a0a674e Compare December 6, 2020 20:37
Copy link
Member

@LordAro LordAro left a 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...

src/fileio.cpp Show resolved Hide resolved
/* 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)));
Copy link
Member

Choose a reason for hiding this comment

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

Why so complex?

Suggested change
std::unique_ptr<char> mem(static_cast<char *>(::operator new(len + 1)));
std::unique_ptr<char[]> mem(new char[len + 1]);

No?

Copy link
Member Author

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()));
Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member Author

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.

Copy link
Member

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?

@michicc
Copy link
Member Author

michicc commented Dec 8, 2020

.c_str() itself is basically a no-op since C++11, but building a std::string from it is somewhat inelegant. I don't know any tool though that will flag that nicely.

@michicc michicc force-pushed the pr/string branch 2 times, most recently from fe1fae3 to 68c82d0 Compare December 13, 2020 17:57
@TrueBrain TrueBrain added needs review: C++ Review requested from a C++ expert candidate: yes This Pull Request is a candidate for being merged size: small This Pull Request is small, and should be relative easy to process labels Dec 14, 2020
LordAro
LordAro previously approved these changes Dec 27, 2020
Copy link
Member

@LordAro LordAro left a 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

@michicc michicc merged commit b408fe7 into OpenTTD:master Dec 27, 2020
@michicc michicc deleted the pr/string branch December 27, 2020 12:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
candidate: yes This Pull Request is a candidate for being merged needs review: C++ Review requested from a C++ expert size: small This Pull Request is small, and should be relative easy to process
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants