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

[René J.V. Bertin]

rjvbb marked 27 inline comments as done.
rjvbb added inline comments.

INLINE COMMENTS

> kfunk wrote in iokitblock.h:43
> Here and below: Missing `Q_DECL_OVERRIDE`

I hope you meant everywhere, including in old code...

> kfunk wrote in iokitdevice.cpp:156
> `new`/`delete` mismatch. Use `delete[]`

(This keeps biting me. Even C doesn't have separate de/allocators for pointers to scalars and pointers to arrays ...)

> kfunk wrote in iokitdevice.cpp:171
> That's *very* odd style. Why does a private class delete its public counterpart? I've never seen this.

Heh, that's because there is (was) a confusion in parenthood here. The member var didn't point to the private class parent, but holds a reference to the parent of the current IOKit device. Currently it's used and thus allocated only when getting the device icon.

> kfunk wrote in iokitdevice.cpp:197
> Don't leave commented code around. Either enable this code paths properly via categorized logging or remove it altogether.

I'd love to, but Solid doesn't have any modern logging set up. Rather than introducing that through the back(end) door, wouldn't it be better if this were done for all of Solid? (Preferably by someone having a good overview of the entire framework...)

https://bugs.kde.org/show_bug.cgi?id=386107

> kfunk wrote in iokitopticaldrive.cpp:27
> Where's that defined via the build system?

Nowhere currently. It's somewhat experimental code which uses the DiskArbitration SDK for ejection, instead of invoking an external (system) command.

It works (and it would thus be a pity to throw everything away) but as documented in the comment, there are a few issues that I haven't been able to iron out.

Making this a build option would certainly increase its visibility and thus (hopefully) incite some other Apple users to test it. Should I put one under the WITH_NEW_* options in the toplevel CMake file?

> kfunk wrote in iokitstorage.cpp:36
> `nullptr`

Did you check that these are indeed pointers? ;)

> kfunk wrote in iokitstorage.cpp:75
> Please just remove the constructors taking a `const IOKitDevice *device` and adapt uses (just use `IOKitDevice *device` everywhere). It's unusual for pointer types to be `const` in such contexts. It just adds noise.

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 ^^)

> kfunk wrote in iokitstorageaccess.cpp:91
> Early-return would reduce the indentation level here [1].
> 
>   if (errorCase)
>     // return early
>     return {};
>   }
>   
>   // normal case
>   // ...
> 
> [1] https://softwareengineering.stackexchange.com/questions/18454/should-i-return-from-a-function-early-or-use-an-if-statement

Compromise. Too many return points make functions hard to maintain (Apple code tends to be full of `goto bail;` instructions because of that; we're far here from the nesting you'd get without those in code using the QT SDK.)

> kfunk wrote in iokitvolume.cpp:177
> That's a lot of copy-pasted code amongst `fsType`, `label`, `vendor`, `product, `description`. I'm sure that can be done better.

Somewhat.

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-mac/attachments/20171023/5c79e9b0/attachment.html>