<table><tr><td style="">kfunk requested changes to this revision.<br />kfunk added inline comments.<br />This revision now requires changes to proceed.
</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-36240" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">iokitdevice.cpp:307</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">default</span><span style="color: #aa2211">:</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">vendor</span> <span style="color: #aa2211">=</span> <span class="n">QString</span><span class="p">();</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">break</span><span class="p">;</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Why not <tt style="background: #ebebeb; font-size: 13px;">return</tt> here as well? For consistency.</p>
<p style="padding: 0; margin: 8px;">You can end the function with</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);">Q_UNREACHABLE();
return {};</pre></div>
<p style="padding: 0; margin: 8px;">Very common pattern.</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-36241" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">iokitdevice.cpp:463</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; "><span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"> <span class="bright"></span><span style="color: #aa4000"><span class="bright">return</span></span><span class="bright"> </span><span class="p"><span class="bright">(</span></span><span class="bright"></span><span class="n"><span class="bright">type</span></span><span class="bright"> </span><span style="color: #aa2211"><span class="bright">==</span></span><span class="bright"> </span><span class="n"><span class="bright">Solid</span></span><span class="bright"></span><span style="color: #aa2211"><span class="bright">::</span></span><span class="bright"></span><span class="n"><span class="bright">DeviceInterface</span></span><span class="bright"></span><span style="color: #aa2211"><span class="bright">::</span></span><span class="bright"></span><span class="n"><span class="bright">GenericInterface</span></span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"> <span class="bright"> </span><span style="color: #aa2211"><span class="bright">||</span></span><span class="bright"> </span><span class="n"><span class="bright">type</span></span><span class="bright"> </span><span style="color: #aa2211"><span class="bright">==</span></span><span class="bright"> </span><span class="n"><span class="bright">d</span></span><span class="bright"></span><span style="color: #aa2211"><span class="bright">-></span></span><span class="n">type</span><span class="p">)<span class="bright">;</span></span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="bright"></span><span style="color: #aa4000"><span class="bright">bool</span></span><span class="bright"> </span><span class="n"><span class="bright">ret</span></span><span class="bright"> </span><span style="color: #aa2211"><span class="bright">=</span></span><span class="bright"> </span><span style="color: #304a96"><span class="bright">false</span></span><span class="bright"></span><span class="p"><span class="bright">;</span></span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="bright"></span><span style="color: #aa4000"><span class="bright">switch</span></span><span class="bright"> </span><span class="p"><span class="bright">(</span></span><span class="n">type</span><span class="p">)<span class="bright"></span></span><span class="bright"> </span><span class="p"><span class="bright">{</span></span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Returning inside the case statements would make this code clearer as well.</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-36245" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">iokitdeviceinterface.h:51</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; "> <span class="n">IOKitDevice</span> <span style="color: #aa2211">*</span><span class="n">m_device</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">IOKitDevice</span> <span style="color: #aa2211">*</span><span class="n">copy</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; "><span class="p">};</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;"><tt style="background: #ebebeb; font-size: 13px;">m_</tt> prefix missing. Would call it <tt style="background: #ebebeb; font-size: 13px;">m_deviceCopy</tt>.</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-36126" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">rjvbb</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;">Did you check that these are indeed pointers? ;)</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Yes. And they are, no?</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-36128" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">rjvbb</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;">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>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Okay, well, leave it like that.</p>
<p style="padding: 0; margin: 8px;">I'm running out of time to properly explain how I'd envision this code to be like in a perfect world.</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-36249" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">iokitstorage.cpp:73</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">const</span> <span class="n">IOKitDevice</span> <span style="color: #aa2211">*</span><span class="n">m_device</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">DASessionRef</span> <span class="n">daSession</span><span class="p">;</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Inconsistent member naming. Some with <tt style="background: #ebebeb; font-size: 13px;">m_</tt> prefix, some without. Choose one style. Private classes' members usually live without the <tt style="background: #ebebeb; font-size: 13px;">m_</tt> prefix, but we don't mind them being around (in KDE land)</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-36250" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">iokitstorage.h:43</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">const</span> <span class="n">QString</span> <span style="color: #004012">vendor</span><span class="p">();</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">const</span> <span class="n">QString</span> <span style="color: #004012">product</span><span class="p">();</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Remove <tt style="background: #ebebeb; font-size: 13px;">const</tt> from return type, but make the method <tt style="background: #ebebeb; font-size: 13px;">const</tt>.</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-36251" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">iokitstorageaccess.cpp:56</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">const</span> <span class="n">IOKitDevice</span> <span style="color: #aa2211">*</span><span class="n">m_device</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">DASessionRef</span> <span class="n">daSession</span><span class="p">;</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Dito, inconsistent member naming.</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-36254" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">iokitvolume.cpp:70</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">const</span> <span class="n">IOKitDevice</span> <span style="color: #aa2211">*</span><span class="n">m_device</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">DASessionRef</span> <span class="n">daSession</span><span class="p">;</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Dito, inconsistent member naming.</p>
<p style="padding: 0; margin: 8px;">And given that I've noticed that three times now, this again leads to the conclusion this is very repetitive code amongst . <tt style="background: #ebebeb; font-size: 13px;">{IOKitStorageAccess,IOKitVolume,IOKitStorage}::Private</tt>.</p>
<p style="padding: 0; margin: 8px;">Maybe there should be helper class for accessing a <tt style="background: #ebebeb; font-size: 13px;">CFDictionary</tt> instead which all these classes use?</p>
<p style="padding: 0; margin: 8px;">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.</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-36252" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">iokitvolume.h:45</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">virtual</span> <span style="color: #aa4000">bool</span> <span style="color: #004012">isIgnored</span><span class="p">()</span> <span style="color: #aa4000">const</span> <span class="n">Q_DECL_OVERRIDE</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">virtual</span> <span class="n">Solid</span><span style="color: #aa2211">::</span><span class="n">StorageVolume</span><span style="color: #aa2211">::</span><span class="n">UsageType</span> <span class="n">usage</span><span class="p">()</span> <span style="color: #aa4000">const</span> <span class="n">Q_DECL_OVERRIDE</span><span class="p">;</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">No <tt style="background: #ebebeb; font-size: 13px;">virtual</tt> needed if there's already a <tt style="background: #ebebeb; font-size: 13px;">Q_DECL_OVERRIDE</tt> or <tt style="background: #ebebeb; font-size: 13px;">override</tt>.</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>