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

Convert PointerWrap::Mode to enum class #10668

Merged

Conversation

Dentomologist
Copy link
Contributor

@Dentomologist Dentomologist commented May 16, 2022

Standard enum class conversion, along with some refactoring to make using PointerWraps a bit more convenient.

  • Added IsReadMode, IsWriteMode, IsMeasureMode, and IsVerifyMode functions so you don't have to list out the full enum value.
  • Likewise added SetMeasureMode and SetVerifyMode. As it stands PointerWraps are never set to Read or Write mode after creation, but more on that subject in a comment below.
  • Removed extraneous comments.
  • Removed unnecessary default enum value.
  • Renamed the mode member variable to m_mode to match normal naming convention.

I've gone with smaller commits for easier reviewing, but I'll squish everything before merging.

@lioncash
Copy link
Member

Looks good. re: the individual points

If the comments are helpful to explain what the modes mean maybe I should just rename the modes? Or if not, delete them.

I'm not that hard pressed either way about this

The values of the modes are arbitrary; I'm inclined to remove the assignment of Mode::Read as it makes it look like the values actually matter.

If all that matters is that the entries have a distinct value, then yeah, the assignment can be removed

Checking which mode a pointerwrap is in is a bit verbose, would IsReadMode() etc. functions be worth it?

Yeah, having dedicated functions to query modes would be nice. May even be able to make the mode enum itself internal doing this as well

@AdmiralCurtiss
Copy link
Contributor

You gonna do more here or should we just merge this for now?

@Dentomologist
Copy link
Contributor Author

Don't merge just yet, I'm in the middle of refactoring some things related to the above conversation.

@Dentomologist Dentomologist marked this pull request as draft May 22, 2022 02:25
@Dentomologist Dentomologist force-pushed the convert_pointerwrap_mode_to_enum_class branch from e6d2299 to 7cb793c Compare May 22, 2022 05:22
@Dentomologist
Copy link
Contributor Author

I made various changes listed in the PR description.

The enum class is still public because its values are used in PointerWrap's constructor, and I haven't found an alternative I'm happy with. Options I considered:

  • Remove the value from the constructor and call SetXMode after, which rather defeats the purpose of a constructor.
  • Setting the mode in derived classes, which introduces new issues and is messier anyway.
  • Making the constructor private and using static factory functions, which I think is frowned on if it's not necessary.

Unless I'm missing something simple I think keeping the status quo here is best.

@Dentomologist Dentomologist marked this pull request as ready for review May 22, 2022 05:47
@Dentomologist Dentomologist force-pushed the convert_pointerwrap_mode_to_enum_class branch from 7cb793c to c8e20c5 Compare May 25, 2022 20:17
if (!IsMeasureMode() && (*m_ptr_current + size) > m_ptr_end)
{
// trying to read/write past the end of the buffer, prevent this
mode = MODE_MEASURE;
SetMeasureMode();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I squashed most of the prior commits.

This pair of changes is new in the squash. I had previously omitted them because I assumed the inline declaration meant it was in an especially critical path and I wanted to avoid adding any function calls, but according to https://github.com/dolphin-emu/dolphin/pull/892/commits that wasn't the case.

@Tilka Tilka merged commit e17a4f4 into dolphin-emu:master May 28, 2022
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants