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

Fixed #11874 (leading 0s ignored by debugger) #8479

Merged
merged 1 commit into from Nov 27, 2019

Conversation

@nokturnusmf
Copy link
Contributor

nokturnusmf commented Nov 19, 2019

Fixes both issues from the report.

For set value:
The original code converts the string into a value and then attempts to work out the size of the input by downcasting and comparing. This breaks for leading zeros as, for example, 0042 == (u8)0042. The fix calculates the correct length from the original string, taking into account the possibility that the string is prefixed by "0x".

For copy hex:
The code already attempts to take the first "length" characters, however incorrectly converts/pads the value and so copies the first "length" nonzero characters. This meant that copying the 01 of 01 23 would result in the string 12 being copied. The fix always fully pads the read value and then correctly takes the leftmost characters.

@@ -493,23 +493,25 @@ void MemoryWidget::OnSetValue()
else
{
bool good_value;
u64 value = m_data_edit->text().toULongLong(&good_value, 16);
const auto& text = m_data_edit->text();
int length = text.startsWith(QStringLiteral("0x")) ? text.size() - 2 : text.size();

This comment has been minimized.

Copy link
@CookiePLMonster

CookiePLMonster Nov 21, 2019

Contributor

What about uppercase 0X?

@@ -493,23 +493,25 @@ void MemoryWidget::OnSetValue()
else
{
bool good_value;
u64 value = m_data_edit->text().toULongLong(&good_value, 16);
const auto& text = m_data_edit->text();

This comment has been minimized.

Copy link
@lioncash

lioncash Nov 21, 2019

Member

text() returns a QString by value, so taking it by const reference, while valid, is somewhat visually misleading.

@@ -493,23 +493,25 @@ void MemoryWidget::OnSetValue()
else
{
bool good_value;
u64 value = m_data_edit->text().toULongLong(&good_value, 16);
const auto& text = m_data_edit->text();
int length = text.startsWith(QStringLiteral("0x")) ? text.size() - 2 : text.size();

This comment has been minimized.

Copy link
@lioncash

lioncash Nov 21, 2019

Member

This and value can both be made const.

@nokturnusmf

This comment has been minimized.

Copy link
Contributor Author

nokturnusmf commented Nov 21, 2019

@CookiePLMonster Yep, complete oversight.

@lioncash So it is, I'm not too familiar with Qt so just assumed.

I additionally noticed and fixed a typo in the code for setting ASCII values, the arguments to WriteU8 were swapped.

@CookiePLMonster

This comment has been minimized.

Copy link
Contributor

CookiePLMonster commented Nov 21, 2019

Looks good now, but please squash so those "PR feedback" commits disappear.

@Helios747 Helios747 merged commit 66ca83e into dolphin-emu:master Nov 27, 2019
8 checks passed
8 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
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.