<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-155971">View Inline</a><span style="color: #4b4d51; font-weight: bold;">kcoreconfigskeleton.h:766</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: #a0a000">public</span><span class="p">:</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">QString</span> <span class="n">value</span><span class="p">()</span> <span style="color: #aa4000">const</span> <span class="p">{</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">It's a struct you can drop the public: here.</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-155972">View Inline</a><span style="color: #4b4d51; font-weight: bold;">kcoreconfigskeleton.h:767</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: #a0a000">public</span><span class="p">:</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">QString</span> <span class="n">value</span><span class="p">()</span> <span style="color: #aa4000">const</span> <span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">return</span> <span class="n">val</span><span class="p">.</span><span class="n">isEmpty</span><span class="p">()</span> <span style="color: #aa2211">?</span> <span style="color: #a0a000">name</span> <span class="p">:</span> <span class="n">val</span><span class="p">;</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">There should be a new line before the opening curly brace.</p>
<p style="padding: 0; margin: 8px;">I also wonder if it wouldn't be better with the implementation being moved to the cpp file, otherwise it risks being inlined which might not be the most convenient for longer term management of the change (if for some reason the implementation needs to be changed).</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-155973">View Inline</a><span style="color: #4b4d51; font-weight: bold;">KConfigCommonStructs.h:61</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: #a0a000">public</span><span class="p">:</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">QString</span> <span class="n">value</span><span class="p">()</span> <span style="color: #aa4000">const</span><span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">return</span> <span class="n">val</span><span class="p">.</span><span class="n">isEmpty</span><span class="p">()</span> <span style="color: #aa2211">?</span> <span style="color: #a0a000">name</span> <span class="p">:</span> <span class="n">val</span><span class="p">;</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">New line before opening curly brace please.</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-155974">View Inline</a><span style="color: #4b4d51; font-weight: bold;">KConfigXmlParser.cpp:203</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(151, 234, 151, .6);"> <span style="color: #aa4000">else</span> <span style="color: #aa4000">if</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">contains</span><span class="p">(</span><span class="n">QLatin1Char</span><span class="p">(</span><span style="color: #766510">' '</span><span class="p">))</span> <span style="color: #aa2211">||</span> <span class="n">choice</span><span class="p">.</span><span class="n">name</span><span class="p">.</span><span class="n">contains</span><span class="p">(</span><span class="n">QLatin1Char</span><span class="p">(</span><span style="color: #766510">'/'</span><span class="p">))</span> <span style="color: #aa2211">||</span> <span class="n">choice</span><span class="p">.</span><span class="n">name</span><span class="p">.</span><span class="n">contains</span><span class="p">(</span><span class="n">QLatin1Char</span><span class="p">(</span><span style="color: #766510">'.'</span><span class="p">))</span> <span style="color: #aa2211">||</span> <span class="n">choice</span><span class="p">.</span><span class="n">name</span><span class="p">.</span><span class="n">contains</span><span class="p">(</span><span class="n">QLatin1Char</span><span class="p">(</span><span style="color: #766510">':'</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;">else if should be just after the closing curly brace</p>
<p style="padding: 0; margin: 8px;">As for checking the valid characters for an enum name this is "easy" it can only be alphabetical, numerical and underscore characters (technically shouldn't start with a digit). Any other character will be rejected by the compiler, the current filter is thus not discriminating enough at all and I think its logic should be reversed (the blacklist being potentially infinite it should be white list based).</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>