<table><tr><td style="">andersonbruce 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/D15521">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/D15521#inline-83798">View Inline</a><span style="color: #4b4d51; font-weight: bold;">jgrulich</span> wrote in <span style="color: #4b4d51; font-weight: bold;">simpleiplistvalidator.cpp:39</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">In both validator constructors (mean also for IPv6 one), you can directly pass "style" param from contructor, given both enums seem to be identical.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">They are the same now but initially they were not (I just added the WithPort option to IPv6) and my thought was that they may not necessarily stay identical in the future. If someone goes in and changes, say, the IPv4 version without realizing it affects the list validator, this code still works.</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/D15521#inline-83804">View Inline</a><span style="color: #4b4d51; font-weight: bold;">pino</span> wrote in <span style="color: #4b4d51; font-weight: bold;">simpleiplistvalidator.cpp:62</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">no need for a regexp, please use <tt style="background: #ebebeb; font-size: 13px;">QString::split()</tt> with <tt style="background: #ebebeb; font-size: 13px;">QString::SkipEmptyParts</tt></p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">I used the regex so that it would split on commas with or without spaces around them and how I read SkipEmptyParts is that it will drop something between two adjacent commas which is not what I was trying to do. <br />
Now I know that I could use "trimmed()" on the resulting strings to strip off any remaining spaces but I don't see any advantage to either using "trimmed()" every time I use one of the resulting strings or doing it once and creating another local copy of the strings for this purpose. It seemed to me that doing everything in one step was a just as viable approach unless there is something inherently bad with splitting on a regex that I am not aware of, especially given that I commented it to explain what it is doing.</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/D15521#inline-83806">View Inline</a><span style="color: #4b4d51; font-weight: bold;">pino</span> wrote in <span style="color: #4b4d51; font-weight: bold;">simpleiplistvalidator.cpp:67</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">set (later on), but never use</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">I don't understand this comment. 'result' is set here to Acceptable, it can be modified to Intermediate at line 102 but if all the addresses are valid it needs to have been set initially to Acceptable for the return at line 106 to be correct.</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/D15521#inline-83811">View Inline</a><span style="color: #4b4d51; font-weight: bold;">pino</span> wrote in <span style="color: #4b4d51; font-weight: bold;">simpleiplisttest.cpp:53</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;">QVERIFY(foo == bar)</tt> -> <tt style="background: #ebebeb; font-size: 13px;">QCOMPARE(foo, bar)</tt>, it applies to all the checks in this file</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Any particular reason you think this is superior?</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R116 Plasma Network Management Applet</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D15521">https://phabricator.kde.org/D15521</a></div></div><br /><div><strong>To: </strong>andersonbruce, jgrulich, pino<br /><strong>Cc: </strong>plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart<br /></div>