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

IOS: Use less ambiguous names for classes #9504

Merged
merged 1 commit into from Feb 13, 2021

Conversation

leoetlino
Copy link
Member

Some of the device names can be ambiguous and require fully or partly
qualifying the name (e.g. IOS::HLE::FS::) in a somewhat verbose way.

Additionally, insufficiently qualified names are prone to breaking.
Consider the example of IOS::HLE::FS:: (namespace) and
IOS::HLE::Device::FS (class). If we use FS::Foo in a file that doesn't
know about the class, everything will work fine. However, as soon as
Device::FS is declared via a header include or even just forward
declared, that code will cease to compile because FS:: now resolves
to Device::FS if FS::Foo was used in the Device namespace.

It also leads to having to write IOS::ES:: to access ES types and
utilities even for code that is already under the IOS namespace.

The fix for this is simple: rename the device classes and give them
a "device" suffix in their names if the existing ones may be ambiguous.
This makes it clear whether we're referring to the device class or to
something else.

This is not any longer to type, considering it lets us get rid of the
Device namespace, which is now wholly unnecessary.

There are no functional changes in this commit.

A future commit will fix unnecessarily qualified names.

Copy link
Contributor

@iwubcode iwubcode left a comment

Choose a reason for hiding this comment

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

Mostly looked at the diff but the few files I checked in full didn't seem to be missing any updated references. LGTM

@iwubcode
Copy link
Contributor

On an unrelated note, I noticed there were a few device classes that could potentially be marked final

@leoetlino
Copy link
Member Author

Yeah, most of them are in older code. That can be done in a follow-up PR -- this one is already large enough :P

Copy link
Member

@BhaaLseN BhaaLseN left a comment

Choose a reason for hiding this comment

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

Pretty verbose diff, but thats expected on such a change. And it reads nicer I'd say, especially when theres a Device class inside a Device namespace and other Device subclasses using the -Device suffix already...
It feels a bit weird that you left the WFS stuff, but I don't insist on the changes if you had good reason to leave them (plus OH0 threw me off before I noticed we already had a OH0Device as well).

@@ -32,8 +32,6 @@ class ARCUnpacker
std::vector<u8> m_whole_file;
};

namespace Device
{
class WFSI : public Device
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to name this one WFSIDevice for consistency?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh whoops, I forgot to rename this. I left the USB classes alone to avoid having everything called a "device" in the USB code (considering we have IOS devices and lots of USB devices there...) For WFS, I don't think there's any particular reason not to use the suffix.

@@ -29,8 +29,6 @@ enum
WFS_FILE_IS_OPENED = -10032, // Cannot perform operation on an opened file.
};

namespace Device
{
class WFSSRV : public Device
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to name this one WFSSRVDevice for consistency?

Some of the device names can be ambiguous and require fully or partly
qualifying the name (e.g. IOS::HLE::FS::) in a somewhat verbose way.

Additionally, insufficiently qualified names are prone to breaking.
Consider the example of IOS::HLE::FS:: (namespace) and
IOS::HLE::Device::FS (class). If we use FS::Foo in a file that doesn't
know about the class, everything will work fine. However, as soon as
Device::FS is declared via a header include or even just forward
declared, that code will cease to compile because FS:: now resolves
to Device::FS if FS::Foo was used in the Device namespace.

It also leads to having to write IOS::ES:: to access ES types and
utilities even for code that is already under the IOS namespace.

The fix for this is simple: rename the device classes and give them
a "device" suffix in their names if the existing ones may be ambiguous.
This makes it clear whether we're referring to the device class or to
something else.

This is not any longer to type, considering it lets us get rid of the
Device namespace, which is now wholly unnecessary.

There are no functional changes in this commit.

A future commit will fix unnecessarily qualified names.
@leoetlino leoetlino merged commit 3e1646a into dolphin-emu:master Feb 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants