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 Dec 6, 2020
Copy link
Member

@LordAro LordAro left a comment

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

@LordAro LordAro Dec 8, 2020

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

@michicc michicc Dec 8, 2020

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

@LordAro LordAro Dec 8, 2020

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

@michicc michicc Dec 8, 2020

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

@michicc michicc Dec 8, 2020

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

@LordAro LordAro Dec 8, 2020

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 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.

Copy link
Member

@LordAro LordAro left a comment

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
8 checks passed
@michicc michicc deleted the pr/string branch Dec 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants