D7401: Solid/Mac : fleshing out the skeleton IOKit backend (WIP)

[Kevin Funk]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20171024/5737dd09/attachment.html>