<table><tr><td style="">ervin requested changes to this revision.<br />ervin 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/D27463">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/D27463#inline-156378">View Inline</a><span style="color: #4b4d51; font-weight: bold;">ervin</span> wrote in <span style="color: #4b4d51; font-weight: bold;">kcoreconfigskeleton.h:764</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">Oh right, stupid me, this obviously breaks binary compatibility, we need to find another way to store this.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Most likely place to hide it would be inside KConfigSkeletonItem's d-pointer (likely a hash associating vals with names)... it'd be unused by most item types of course which sucks but at least it'd be safe BC wise.</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/D27463#inline-156379">View Inline</a><span style="color: #4b4d51; font-weight: bold;">KConfigXmlParser.cpp:202</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">std</span><span style="color: #aa2211">::</span><span class="n">cerr</span> <span style="color: #aa2211"><<</span> <span style="color: #766510">"Tag <choice> requires attribute 'name'."</span> <span style="color: #aa2211"><<</span> <span class="n">std</span><span style="color: #aa2211">::</span><span class="n">endl</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">        <span class="p">}</span> <span style="color: #aa4000">else</span> <span style="color: #aa4000">if</span> <span class="p">(</span><span style="color: #aa2211">!</span><span class="p">(</span><span class="n">QRegularExpression</span><span class="p">(</span><span class="n">QStringLiteral</span><span class="p">(</span><span style="color: #766510">"</span><span style="color: #bb6622">\\</span><span style="color: #766510">w+"</span><span class="p">))).</span><span class="n">match</span><span class="p">(</span><span class="n">choice</span><span class="p">.</span><span class="n">name</span><span class="p">).</span><span class="n">hasMatch</span><span class="p">())</span> <span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">            <span class="n">std</span><span style="color: #aa2211">::</span><span class="n">cerr</span> <span style="color: #aa2211"><<</span> <span style="color: #766510">"Tag <choice> attribute 'name' must be compatible with Enum naming. name was '"</span> <span style="color: #aa2211"><<</span> <span class="n">qPrintable</span><span class="p">(</span><span class="n">choice</span><span class="p">.</span><span class="n">name</span><span class="p">)</span> <span style="color: #aa2211"><<</span> <span style="color: #766510">"'. You can use attribute 'value' to pass any string as the choice value."</span> <span style="color: #aa2211"><<</span> <span class="n">std</span><span style="color: #aa2211">::</span><span class="n">endl</span><span class="p">;</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">We should move the QRegularExpression out of the loop, otherwise it'll get compiled over and over for each choice (premature pessimisation imo). We could go one step further and have it as a member of the parser to have it compiled only once of course, but maybe we'd get in premature optimization territory.</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R237 KConfig</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D27463">https://phabricator.kde.org/D27463</a></div></div><br /><div><strong>To: </strong>meven, ervin, bport, crossi, Frameworks<br /><strong>Cc: </strong>ngraham, davidre, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, bruns<br /></div>