kfunk requested changes to this revision.
kfunk added inline comments.
This revision now requires changes to proceed.
INLINE COMMENTS
> iokitdevice.cpp:307
> + default:
> + vendor = QString();
> + break;
Why not `return` here as well? For consistency.
You can end the function with
Q_UNREACHABLE();
return {};
Very common pattern.
> iokitdevice.cpp:463
> {
> - return (type == Solid::DeviceInterface::GenericInterface
> - || type == d->type);
> + bool ret = false;
> + switch (type) {
Returning inside the case statements would make this code clearer as well.
> iokitdeviceinterface.h:51
> IOKitDevice *m_device;
> + IOKitDevice *copy;
> };
`m_` prefix missing. Would call it `m_deviceCopy`.
> rjvbb wrote in iokitstorage.cpp:36
> Did you check that these are indeed pointers? ;)
Yes. And they are, no?
> rjvbb wrote in iokitstorage.cpp:75
> I don't get it, sorry, can you explain in more words how you'd want to see
> this changed? If I remove this extra ctor, I can no longer call
> `IOKitStorage(this).vendor()` in `IOKitDevice::vendor()` without extra code
> that's also going to add noise.
>
> I get a warning when I remove the const attribute from
> `IOKitDevice::vendor()`, which suggests that I'd no longer be reimplementing
> a virtual method but adding a method instead.
>
> All these "extra" ctors hand off the pointer to a "const this" to
> `DeviceInterface` which finally makes a deep copy. I find that cleaner than
> creating a deep copy of `this` everywhere needed and ensuring it gets
> deallocated (even via QPointers).
> Unusual doesn't mean wrong (we're on Mac here, after all ^^)
Okay, well, leave it like that.
I'm running out of time to properly explain how I'd envision this code to be
like in a perfect world.
> iokitstorage.cpp:73
> +
> + const IOKitDevice *m_device;
> + DASessionRef daSession;
Inconsistent member naming. Some with `m_` prefix, some without. Choose one
style. Private classes' members usually live without the `m_` prefix, but we
don't mind them being around (in KDE land)
> iokitstorage.h:43
> +
> + const QString vendor();
> + const QString product();
Remove `const` from return type, but make the method `const`.
> iokitstorageaccess.cpp:56
> +
> + const IOKitDevice *m_device;
> + DASessionRef daSession;
Dito, inconsistent member naming.
> iokitvolume.cpp:70
> +
> + const IOKitDevice *m_device;
> + DASessionRef daSession;
Dito, inconsistent member naming.
And given that I've noticed that three times now, this again leads to the
conclusion this is very repetitive code amongst .
`{IOKitStorageAccess,IOKitVolume,IOKitStorage}::Private`.
Maybe there should be helper class for accessing a `CFDictionary` instead which
all these classes use?
I'm not trying to piss you off René, but this is slightly sloppy coding which
easy for the initial writer to do, but will bite us any time in the future when
someone unfamiliar with the code needs to do fixes to this code and potentially
fixes only one copy of these snippets. Please think about your architecture
more carefully.
> iokitvolume.h:45
> +
> + virtual bool isIgnored() const Q_DECL_OVERRIDE;
> + virtual Solid::StorageVolume::UsageType usage() const Q_DECL_OVERRIDE;
No `virtual` needed if there's already a `Q_DECL_OVERRIDE` or `override`.
REPOSITORY
R245 Solid
REVISION DETAIL
https://phabricator.kde.org/D7401
To: rjvbb, #frameworks, kfunk
Cc: kfunk, anthonyfieroni, cgilles, kde-mac
