<table><tr><td style="">rjvbb marked 10 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-36241" 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:463</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">Returning inside the case statements would make this code clearer as well.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">I don't share that opinion and think that multiple exit points do not make the code more efficient either (judging from stepping through the code in a debugger).</p>

<p style="padding: 0; margin: 8px;">But whatever...</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;">kfunk</span> wrote in <span style="color: #4b4d51; font-weight: bold;">iokitvolume.cpp:70</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><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>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Not really certain why I didn't decide to this myself, I'm pretty certain it did cross my mind.</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;">kfunk</span> wrote in <span style="color: #4b4d51; font-weight: bold;">iokitvolume.h:45</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><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 style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">I guess that's safe here because IOKitOpticalDisc doesn't override any of these methods?</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>ngraham, kfunk, anthonyfieroni, cgilles, kde-mac<br /></div>