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

Change a bunch of stuff to use std::string #8164

Merged
merged 12 commits into from May 21, 2020
Merged

Conversation

michicc
Copy link
Member

@michicc michicc commented May 19, 2020

This PR converts a more or less random selection of stuff to use std::string instead of manually managed strings. It is absolutely not complete in any sense, but then I don't think there's any point in trying to convert the whole source code in a massive effort at once.

Selection which parts to convert were basically made by randomly picking a starting point and then following the tendrils that go from there.

I've already continued with some other source code parts, so expect some more PRs.

Copy link
Contributor

@Yexo Yexo left a comment

https://github.com/OpenTTD/OpenTTD/blob/master/README.md#30-licensing explicitly calls out a few exceptions to the default licensing. I think you should add a bit there about this new code.

Edit: the above was meant to be about fc190ac.

@glx22
Copy link
Contributor

@glx22 glx22 commented May 19, 2020

Hmm crash during regression check, and macos doesn't build.

@michicc
Copy link
Member Author

@michicc michicc commented May 19, 2020

MacOS is easy and just a failure to compile. The regression seems to crash somewhere, have to investigate that.

@Yexo I never checked that part of the readme actually, will add something.

src/ini_type.h Show resolved Hide resolved
src/newgrf_config.h Outdated Show resolved Hide resolved
src/string.cpp Show resolved Hide resolved
src/game/game_text.hpp Show resolved Hide resolved
src/company_base.h Show resolved Hide resolved
@@ -28,78 +28,81 @@
<Filter Include="MD5">
<UniqueIdentifier>{c76ff9f1-1e62-46d8-8d55-000000000008}</UniqueIdentifier>
</Filter>
<Filter Include="Script">
<Filter Include="Compat">
Copy link
Contributor

@nielsmh nielsmh May 19, 2020

Choose a reason for hiding this comment

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

Why have all of these filter/group names switched around... weird.

Copy link
Contributor

@glx22 glx22 May 19, 2020

Choose a reason for hiding this comment

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

Generate script uses alphabetical order. But it's weird MD5 is above.

Ah no, it just use the first seen order.

Copy link
Member Author

@michicc michicc May 20, 2020

Choose a reason for hiding this comment

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

It is also a diff algorithm artifact. A better diff would simply show Compat being inserted after MD5 instead of all this single line shifting.

Copy link
Contributor

@nielsmh nielsmh May 20, 2020

Choose a reason for hiding this comment

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

I guess it's caused by all the UniqueIdentifier values getting renumbered.

src/gfxinit.cpp Show resolved Hide resolved
LordAro
Copy link
Member

@LordAro LordAro commented on 73409ec May 19, 2020

Choose a reason for hiding this comment

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

std::make_unique ? (Probably other places too, just noticed this one)

michicc
Copy link
Member Author

@michicc michicc commented on 73409ec May 19, 2020

Choose a reason for hiding this comment

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

It's C++14...

@michicc michicc merged commit c972a63 into OpenTTD:master May 21, 2020
8 checks passed
@michicc michicc deleted the pr/string_1 branch May 21, 2020
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.

None yet

6 participants