<table><tr><td style="">rjvbb marked 27 inline comments as done.<br />rjvbb added inline comments.
</td><a style="text-decoration: none; padding: 4px 8px; margin: 0 8px 8px; float: right; color: #464C5C; font-weight: bold; border-radius: 3px; background-color: #F7F7F9; background-image: linear-gradient(to bottom,#fff,#f1f0f1); display: inline-block; border: 1px solid rgba(71,87,120,.2);" href="https://phabricator.kde.org/D7401" rel="noreferrer">View Revision</a></tr></table><br /><div><strong>INLINE COMMENTS</strong><div><div style="margin: 6px 0 12px 0;"><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D7401#inline-36023" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">kfunk</span> wrote in <span style="color: #4b4d51; font-weight: bold;">iokitblock.h:43</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">Here and below: Missing <tt style="background: #ebebeb; font-size: 13px;">Q_DECL_OVERRIDE</tt></p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">I hope you meant everywhere, including in old code...</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D7401#inline-36025" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">kfunk</span> wrote in <span style="color: #4b4d51; font-weight: bold;">iokitdevice.cpp:156</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;"><tt style="background: #ebebeb; font-size: 13px;">new</tt>/<tt style="background: #ebebeb; font-size: 13px;">delete</tt> mismatch. Use <tt style="background: #ebebeb; font-size: 13px;">delete[]</tt></p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">(This keeps biting me. Even C doesn't have separate de/allocators for pointers to scalars and pointers to arrays ...)</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D7401#inline-36029" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">kfunk</span> wrote in <span style="color: #4b4d51; font-weight: bold;">iokitdevice.cpp:171</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">That's *very* odd style. Why does a private class delete its public counterpart? I've never seen this.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">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.</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D7401#inline-36030" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">kfunk</span> wrote in <span style="color: #4b4d51; font-weight: bold;">iokitdevice.cpp:197</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">Don't leave commented code around. Either enable this code paths properly via categorized logging or remove it altogether.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">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...)</p>

<p style="padding: 0; margin: 8px;"><a href="https://bugs.kde.org/show_bug.cgi?id=386107" class="remarkup-link" target="_blank" rel="noreferrer">https://bugs.kde.org/show_bug.cgi?id=386107</a></p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D7401#inline-36060" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">kfunk</span> wrote in <span style="color: #4b4d51; font-weight: bold;">iokitopticaldrive.cpp:27</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">Where's that defined via the build system?</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Nowhere currently. It's somewhat experimental code which uses the DiskArbitration SDK for ejection, instead of invoking an external (system) command.</p>

<p style="padding: 0; margin: 8px;">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.</p>

<p style="padding: 0; margin: 8px;">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?</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D7401#inline-36043" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">kfunk</span> wrote in <span style="color: #4b4d51; font-weight: bold;">iokitstorage.cpp:36</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;"><tt style="background: #ebebeb; font-size: 13px;">nullptr</tt></p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Did you check that these are indeed pointers? ;)</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D7401#inline-36045" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">kfunk</span> wrote in <span style="color: #4b4d51; font-weight: bold;">iokitstorage.cpp:75</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">Please just remove the constructors taking a <tt style="background: #ebebeb; font-size: 13px;">const IOKitDevice *device</tt> and adapt uses (just use <tt style="background: #ebebeb; font-size: 13px;">IOKitDevice *device</tt> everywhere). It's unusual for pointer types to be <tt style="background: #ebebeb; font-size: 13px;">const</tt> in such contexts. It just adds noise.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">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 <tt style="background: #ebebeb; font-size: 13px;">IOKitStorage(this).vendor()</tt> in <tt style="background: #ebebeb; font-size: 13px;">IOKitDevice::vendor()</tt> without extra code that's also going to add noise.</p>

<p style="padding: 0; margin: 8px;">I get a warning when I remove the const attribute from <tt style="background: #ebebeb; font-size: 13px;">IOKitDevice::vendor()</tt>, which suggests that I'd no longer be reimplementing a virtual method but adding a method instead.</p>

<p style="padding: 0; margin: 8px;">All these "extra" ctors hand off the pointer to a "const this" to <tt style="background: #ebebeb; font-size: 13px;">DeviceInterface</tt> which finally makes a deep copy. I find that cleaner than creating a deep copy of <tt style="background: #ebebeb; font-size: 13px;">this</tt> everywhere needed and ensuring it gets deallocated (even via QPointers).<br />
Unusual doesn't mean wrong (we're on Mac here, after all ^^)</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D7401#inline-36048" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">kfunk</span> wrote in <span style="color: #4b4d51; font-weight: bold;">iokitstorageaccess.cpp:91</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">Early-return would reduce the indentation level here [1].</p>

<div class="remarkup-code-block" style="margin: 12px 0;" data-code-lang="text" data-sigil="remarkup-code-block"><pre class="remarkup-code" style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; padding: 12px; margin: 0; background: rgba(71, 87, 120, 0.08);">if (errorCase)
  // return early
  return {};
}

// normal case
// ...</pre></div>

<p style="padding: 0; margin: 8px;">[1] <a href="https://softwareengineering.stackexchange.com/questions/18454/should-i-return-from-a-function-early-or-use-an-if-statement" class="remarkup-link" target="_blank" rel="noreferrer">https://softwareengineering.stackexchange.com/questions/18454/should-i-return-from-a-function-early-or-use-an-if-statement</a></p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Compromise. Too many return points make functions hard to maintain (Apple code tends to be full of <tt style="background: #ebebeb; font-size: 13px;">goto bail;</tt> instructions because of that; we're far here from the nesting you'd get without those in code using the QT SDK.)</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D7401#inline-36055" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">kfunk</span> wrote in <span style="color: #4b4d51; font-weight: bold;">iokitvolume.cpp:177</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">That's a lot of copy-pasted code amongst <tt style="background: #ebebeb; font-size: 13px;">fsType</tt>, <tt style="background: #ebebeb; font-size: 13px;">label</tt>, <tt style="background: #ebebeb; font-size: 13px;">vendor</tt>, <tt style="background: #ebebeb; font-size: 13px;">product, </tt>description`. I'm sure that can be done better.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Somewhat.</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R245 Solid</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D7401" rel="noreferrer">https://phabricator.kde.org/D7401</a></div></div><br /><div><strong>To: </strong>rjvbb, Frameworks, kfunk<br /><strong>Cc: </strong>kfunk, anthonyfieroni, cgilles, kde-mac<br /></div>