D15521: Add validator for lists of IP addressesAdded as separate review per comment from Pino onreview D15093. This code will not compile withoutthe updated code in review D15520. Also includesunit test.

[Pino Toscano]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20180917/714c807f/attachment-0001.html>