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

Fix TitleDatabase #8307

Merged
merged 1 commit into from Aug 10, 2019

Conversation

@JosJuice
Copy link
Contributor

commented Aug 9, 2019

a2a1e04 regression.

Fix TitleDatabase
a2a1e04 regression.

@JosJuice JosJuice force-pushed the JosJuice:fix-titledatabase branch from fa2b282 to 9cc719d Aug 9, 2019

@stenzek
Copy link
Contributor

left a comment

LGTM

@stenzek stenzek merged commit dd8bc7b into dolphin-emu:master Aug 10, 2019

10 checks passed

default Very basic checks passed, handed off to Buildbot.
Details
lint Build succeeded on builder lint
Details
pr-android Build succeeded on builder pr-android
Details
pr-deb-dbg-x64 Build succeeded on builder pr-deb-dbg-x64
Details
pr-deb-x64 Build succeeded on builder pr-deb-x64
Details
pr-freebsd-x64 Build succeeded on builder pr-freebsd-x64
Details
pr-osx-x64 Build succeeded on builder pr-osx-x64
Details
pr-ubu-x64 Build succeeded on builder pr-ubu-x64
Details
pr-win-dbg-x64 Build succeeded on builder pr-win-dbg-x64
Details
pr-win-x64 Build succeeded on builder pr-win-x64
Details

@JosJuice JosJuice deleted the JosJuice:fix-titledatabase branch Aug 10, 2019

@dreamsyntax

This comment has been minimized.

Copy link
Contributor

commented Aug 10, 2019

Is there a reason this regression was done?

Prior to this commit the Title field would be pulled from the game's DOL.
This is preferred when dealing with multiple modified copies of a game in the list.
@JosJuice

@JosJuice

This comment has been minimized.

Copy link
Contributor Author

commented Aug 10, 2019

Is there a reason this regression was done?

The regression was because I overlooked that the temporary std::string gets destroyed before we use the std::string_view again. (Most likely I applied less thinking and testing to it than I usually would since the change to std::string_view was done in response to a review comment.) Nothing more than a simple mistake.

Prior to this commit the Title field would be pulled from the game's DOL.

The DOL doesn't have a title field. I'm assuming you mean the opening.bnr file. You can get the old behavior back by turning off the setting Config > Interface > Use Built-In Database of Game Names.

@CookiePLMonster

This comment has been minimized.

Copy link
Contributor

commented Aug 10, 2019

Does it truly solve the issue? Right now it relies on line_view which would also get invalid as soon as line is destroyed. Or is it that StripSpaces on std::string returns a std::string and now it's invoking it on std::string_view?

@JosJuice

This comment has been minimized.

Copy link
Contributor Author

commented Aug 10, 2019

Does it truly solve the issue? Right now it relies on line_view which would also get invalid as soon as line is destroyed.

We only need line_view to be valid until we've done the emplace (because the emplace converts to std::string when inserting into the map), so it becoming invalid after that is fine.

Or is it that StripSpaces on std::string returns a std::string and now it's invoking it on std::string_view?

No, it always returns an std::string_view. (If it had returned an std::string we wouldn't have had any bug to begin with.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants
You can’t perform that action at this time.