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

Reply via email to