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: A little cleanup. #8019

Open
wants to merge 12 commits into
base: master
from

Conversation

5 participants
@AdmiralCurtiss
Copy link
Contributor

commented Apr 21, 2019

Cleans up the memory card code a bit more. Tries to get rid of undefined behavior, updates naming conventions, moves code out of the header into the code file, converts enum values into enum classes or constexpr as appropriate. Should have zero functional changes.

@AdmiralCurtiss AdmiralCurtiss force-pushed the AdmiralCurtiss:gcmemcard-header-cleanup branch from e89bbce to 835731e Apr 21, 2019

@lioncash

This comment has been minimized.

Copy link
Member

commented Apr 21, 2019

As a sidenote, is some of this stuff the code is doing even legal in modern C++?

Legal? Sure. Free of invoking undefined behavior? Absolutely not. Even in pre-modern C++.

@AdmiralCurtiss

This comment has been minimized.

Copy link
Contributor Author

commented Apr 21, 2019

Yeah, I thought so. Should probably clean that up then while I'm here.

@AdmiralCurtiss AdmiralCurtiss force-pushed the AdmiralCurtiss:gcmemcard-header-cleanup branch from 835731e to 33b4801 Apr 21, 2019

@AdmiralCurtiss AdmiralCurtiss force-pushed the AdmiralCurtiss:gcmemcard-header-cleanup branch from 33b4801 to 0a0f55e Apr 21, 2019

@BhaaLseN
Copy link
Member

left a comment

Do those structured bindings even work on all of our supported compilers yet? Not to mention that they're somewhat inconsistent in the way they're formatted.

// TODO: determine the purpose of m_unknown_2
// 1 works for slot A, 0 works for both slot A and slot B
memset(m_unknown_2.data(), 0,
m_unknown_2.size()); // = _viReg[55]; static vu16* const _viReg = (u16*)0xCC002000;

This comment has been minimized.

Copy link
@BhaaLseN

BhaaLseN Apr 21, 2019

Member

This comment has just been moved along, but do we know what it means?

This comment has been minimized.

Copy link
@AdmiralCurtiss

AdmiralCurtiss Apr 21, 2019

Author Contributor

Not sure? I guess it's meant to imply that it copies the value from the VI_DTV_STATUS register, but I don't think that makes a lot of sense.

{
rand = (((rand * (u64)0x0000000041c64e6dULL) + (u64)0x0000000000003039ULL) >> 16);
m_serial[i] = (u8)(g_SRAM.settings_ex.flash_id[slot][i] + (u32)rand);
rand = (((rand * (u64)0x0000000041c64e6dULL) + (u64)0x0000000000003039ULL) >> 16);

This comment has been minimized.

Copy link
@BhaaLseN

BhaaLseN Apr 21, 2019

Member

Any point in making those constexpr? Not sure what name to give them tho.

Show resolved Hide resolved Source/Core/Core/HW/GCMemcard/GCMemcard.cpp Outdated

@AdmiralCurtiss AdmiralCurtiss force-pushed the AdmiralCurtiss:gcmemcard-header-cleanup branch from 0a0f55e to 93e40a4 Apr 21, 2019

@AdmiralCurtiss

This comment has been minimized.

Copy link
Contributor Author

commented Apr 21, 2019

By the by, have we ever considered Dolphin support on big endian platforms? A decent amount of code in this class assumes native little endian.

@AdmiralCurtiss

This comment has been minimized.

Copy link
Contributor Author

commented Apr 21, 2019

Should we also modify the constructors to use standard initialization instead of memset? I started doing that but stopped myself after noticing that a one-liner memset(this, 0xFF, DENTRY_SIZE) transformed into this thing:

DEntry::DEntry()
    : m_unused_1(0xFFu), m_banner_and_icon_flags(0xFFu), m_modification_time(0xFFFFFFFFu),
      m_image_offset(0xFFFFFFFFu), m_icon_format(0xFFFFu), m_animation_speed(0xFFFFu),
      m_file_permissions(0xFFu), m_copy_counter(0xFFu), m_first_block(0xFFFFu),
      m_block_count(0xFFFFu), m_comments_address(0xFFFFFFFFu)
{
  m_gamecode.fill(0xFF);
  m_makercode.fill(0xFF);
  m_filename.fill(0xFF);
  m_unused_2.fill(0xFF);
}

And as far as I can tell memset is totally fine for trivially copyable types.

@BhaaLseN

This comment has been minimized.

Copy link
Member

commented Apr 22, 2019

It probably becomes a lot less verbose if you just in-class initialize them:

struct DEntry
{
  DEntry();
  std::string GCI_FileName() const;
  static constexpr std::array<u8, 4> UNINITIALIZED_GAMECODE{{0xFF, 0xFF, 0xFF, 0xFF}};

  std::array<u8, 4> m_gamecode{{ 0xFF, 0xFF, 0xFF, 0xFF }};
  std::array<u8, 2> m_makercode{{ 0xFF, 0xFF }};
  u8 m_unused_1 = 0xFF;
  u8 m_banner_and_icon_flags = 0xFF;
  // etc...
};

Kinda +/-0 on that though...

@AdmiralCurtiss AdmiralCurtiss force-pushed the AdmiralCurtiss:gcmemcard-header-cleanup branch 3 times, most recently from 686da3b to 00eccc9 May 4, 2019

@AdmiralCurtiss

This comment has been minimized.

Copy link
Contributor Author

commented May 5, 2019

Regarding commit eea5a30, I find it a bit weird how the code goes through great pain to return a somewhat descriptive return code but all the calling code ever only checks for SUCCESS. Should we simplify the code here to just return a true/false, or should we give better error messages on the outside?

@AdmiralCurtiss AdmiralCurtiss force-pushed the AdmiralCurtiss:gcmemcard-header-cleanup branch from 00eccc9 to 56e1dc4 May 5, 2019

@JMC47

This comment has been minimized.

Copy link
Contributor

commented May 18, 2019

Any update on this? I don't see any approvals on it so I don't want to merge it, but, our GC memcard code probably needs the cleanups based on what I know of it.

Show resolved Hide resolved Source/Core/Core/HW/GCMemcard/GCMemcard.cpp Outdated

@AdmiralCurtiss AdmiralCurtiss force-pushed the AdmiralCurtiss:gcmemcard-header-cleanup branch from 56e1dc4 to f6eaee3 May 18, 2019

@AdmiralCurtiss

This comment has been minimized.

Copy link
Contributor Author

commented May 18, 2019

From my side I think this is good to go, there's still sketchy stuff in here but nothing that wasn't already sketchy before, and I think adding more on top of this would just make it harder to review. Let's refactor this step by step.

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