pino requested changes to this revision. pino added a comment. This revision now requires changes to proceed.
- same note as in D15520: Upgrade SimpleIpV4AddressValidator and SimpleIpV6AddressValidator <https://phabricator.kde.org/D15520> regarding the commit, and the summary of this review - in the test, better use the data-driven approach for tests, instead of long copy&paste lines of "set data, check result" sequences INLINE COMMENTS > simpleiplistvalidator.cpp:29 > + AddressType type) > + : QValidator(parent) > +{ please set both `m_ipv4Validator` and `m_ipv6Validator`, otherwise they can be left uninitialized when `type` is not `Both` > simpleiplistvalidator.cpp:62 > + // Split the incoming address on commas possibly with spaces on either > side > + QStringList addressList = address.split(QRegularExpression("\\s*,\\s*")); > + no need for a regexp, please use `QString::split()` with `QString::SkipEmptyParts` > simpleiplistvalidator.cpp:67 > + int localPos = 0; > + int i = 0; > + QValidator::State result = QValidator::Acceptable; set (later on), but never use > simpleiplistvalidator.cpp:101 > + // that's the default set on entry and we only downgrade it from > there. > + if (result != QValidator::Invalid > + && (ipv4Result == QValidator::Intermediate || ipv6Result == > QValidator::Intermediate)) `result` will never be `QValidator::Invalid`, so this condition is always true > CMakeLists.txt:13-15 > +add_executable(simpleipv6test ${simpleipv6test_SRCS}) > +add_test(simpleipv6 simpleipv6test) > +ecm_mark_as_test(simpleipv6test) just use `ecm_add_test` instead > CMakeLists.txt:26-28 > +add_executable(simpleipv4test ${simpleipv4test_SRCS}) > +add_test(simpleipv4 simpleipv4test) > +ecm_mark_as_test(simpleipv4test) ditto > CMakeLists.txt:42-44 > +add_executable(simpleiplisttest ${simpleiplisttest_SRCS}) > +add_test(simpleiplist simpleiplisttest) > +ecm_mark_as_test(simpleiplisttest) ditto > simpleiplisttest.cpp:23 > +#include <QTest> > +#include <QDir> > + unused > simpleiplisttest.cpp:43-45 > + vb = new SimpleIpListValidator(nullptr, > SimpleIpListValidator::AddressStyle::Base); > + vc = new SimpleIpListValidator(nullptr, > SimpleIpListValidator::AddressStyle::WithCidr); > + vp = new SimpleIpListValidator(nullptr, > SimpleIpListValidator::AddressStyle::WithPort); even if this is a test, these are leaked, "polluting" the results of tools like valgrind; just make them non-pointers class variables > simpleiplisttest.cpp:53 > + QString tstr(""); > + QVERIFY(vb->validate(tstr, pos) == QValidator::Intermediate); > + `QVERIFY(foo == bar)` -> `QCOMPARE(foo, bar)`, it applies to all the checks in this file REPOSITORY R116 Plasma Network Management Applet REVISION DETAIL https://phabricator.kde.org/D15521 To: andersonbruce, jgrulich, pino Cc: plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
