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

GCMemcard: Remove byteswapping functions and macros #8501

Open
wants to merge 2 commits into
base: master
from

Conversation

@lioncash
Copy link
Member

lioncash commented Nov 28, 2019

ByteSwap can just be replaced with std::swap, and rather than use preprocessor macros for the swap functions, we can just use them directly.

lioncash added 2 commits Nov 28, 2019
There's already a standard library function that does what this function
is doing.
We can just specify the functions directly instead of relying on
preprocessor textual replacement.
@@ -166,7 +166,9 @@ void CEXIMemoryCard::SetupGciFolder(u16 sizeMb)
u32 CurrentGameId = 0;
if (game_id.length() >= 4 && game_id != "00000000" &&
SConfig::GetInstance().GetTitleID() != Titles::SYSTEM_MENU)
CurrentGameId = BE32((u8*)game_id.c_str());
{
CurrentGameId = Common::swap32(reinterpret_cast<const u8*>(game_id.c_str()));

This comment has been minimized.

Copy link
@CookiePLMonster

CookiePLMonster Nov 28, 2019

Contributor

How does this work? Swapping endianness of a pointer?

This comment has been minimized.

Copy link
@lioncash

lioncash Nov 28, 2019

Author Member

The swap functions have overloads for operating on the data pointed to by u8 pointers.

This comment has been minimized.

Copy link
@CookiePLMonster

CookiePLMonster Nov 28, 2019

Contributor

Oh, that is a bit counter-intuitive - so this in fact extracts first 4 characters of a string and byteswaps them, right?

This comment has been minimized.

Copy link
@lioncash

lioncash Nov 28, 2019

Author Member

Yes.

This comment has been minimized.

Copy link
@CookiePLMonster

CookiePLMonster Nov 28, 2019

Contributor

Cool - that said, I would personally prefer to see those not overloaded but rather as a family of swapPtrXX functions. I feel like right now it is needlessly easy to confuse swap32(T) and swap32(T*).

ByteSwap(&tmp[2], &tmp[3]);
memcpy(&tempDEntry.m_image_offset, tmp.data(), 4);
std::memcpy(tmp.data(), &tempDEntry.m_image_offset, 4);
std::swap(tmp[0], tmp[1]);

This comment has been minimized.

Copy link
@CookiePLMonster

CookiePLMonster Nov 28, 2019

Contributor

This isn't a big endian to little endian swap for sure, so what is it?

This comment has been minimized.

Copy link
@AdmiralCurtiss

AdmiralCurtiss Nov 28, 2019

Contributor

I'm not 100% sure as this isn't spelled out, but I suspect this happens because some thirdparty Memory Card dumping hardware (Datel's MAX Drive, I think?) reads these fields in 16 bits words of the wrong endianness. But either way, that's just how the SAV format stores its data.

This comment has been minimized.

Copy link
@CookiePLMonster

CookiePLMonster Nov 28, 2019

Contributor

Fair enough, just wanted to point it out in an unlikely chance of a typo/misworded intention.

This comment has been minimized.

Copy link
@BhaaLseN

BhaaLseN Nov 28, 2019

Member

Is this worth updating the comment above to include a (speculative?) reason why this is? It does say "swap byte pairs", but gives no reason for it.

@CookiePLMonster

This comment has been minimized.

Copy link
Contributor

CookiePLMonster commented Nov 28, 2019

Less macros = more good, so always happy to see such changes!

*(u16*)&FileBuffer[1].m_block[0x0066] = BE16(BE32(serial1) >> 16);
*(u16*)&FileBuffer[3].m_block[0x1580] = BE16(BE32(serial2) >> 16);
*(u16*)&FileBuffer[1].m_block[0x0060] = BE16(BE32(serial1) & 0xFFFF);
*(u16*)&FileBuffer[1].m_block[0x0200] = BE16(BE32(serial2) & 0xFFFF);
Comment on lines -1436 to -1439

This comment has been minimized.

Copy link
@AdmiralCurtiss

AdmiralCurtiss Nov 28, 2019

Contributor

Type punning on assignment like that is undefined behavior, isn't it? Even if it isn't this looks pretty sketchy, maybe memcpy those bytes into the m_blocks instead?

This comment has been minimized.

Copy link
@lioncash

lioncash Nov 28, 2019

Author Member

I'll save resolving this for a follow-up PR, given there's several other locations within the source file where this sort of type-punning is performed, and I'd rather resolve them all at once.

This comment has been minimized.

Copy link
@AdmiralCurtiss

AdmiralCurtiss Nov 30, 2019

Contributor

Mhm, fair enough. Looks fine to me then.

u16 BlockCount = DEntry_BlockCount(index);
// u16 memcardSize = BE16(hdr.m_size_mb) * MBIT_TO_BLOCKS;
const u16 block = DEntry_FirstBlock(index);
const u16 BlockCount = DEntry_BlockCount(index);

This comment has been minimized.

Copy link
@BhaaLseN

BhaaLseN Nov 28, 2019

Member

Wanna update this ones' naming convention while we're at it?

ByteSwap(&tmp[2], &tmp[3]);
memcpy(&tempDEntry.m_image_offset, tmp.data(), 4);
std::memcpy(tmp.data(), &tempDEntry.m_image_offset, 4);
std::swap(tmp[0], tmp[1]);

This comment has been minimized.

Copy link
@BhaaLseN

BhaaLseN Nov 28, 2019

Member

Is this worth updating the comment above to include a (speculative?) reason why this is? It does say "swap byte pairs", but gives no reason for it.

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.