Upgrade SimpleIpV4AddressValidator and SimpleIpV6AddressValidator
ClosedPublic

Authored by andersonbruce on Sep 15 2018, 2:18 AM.

Details

Summary

to optionally handle a CIDR or port suffix. Added capabilitiesimplemented via defaulted parameter to maintain compatibilitywith the previous version. Includes unit tests for the updated
code. Opened as a separate review per comment from Pino on review
D15093.

Part of:
CCBUG: 397572
FIXED-IN: 5.14.0

Diff Detail

Repository
R116 Plasma Network Management Applet
Branch
IP4/6validatorsUpdate
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 3194
Build 3212: arc lint + arc unit
andersonbruce created this revision.Sep 15 2018, 2:18 AM
Restricted Application added a project: Plasma. · View Herald TranscriptSep 15 2018, 2:18 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
andersonbruce requested review of this revision.Sep 15 2018, 2:18 AM

That huge summary needs reformatting, please. :) See https://chris.beams.io/posts/git-commit/#seven-rules

andersonbruce retitled this revision from Upgrade SimpleIpV4AddressValidator and SimpleIpV6AddressValidator to optionally handle a CIDR or port suffix. Added capabilities implemented via defaulted parameter to maintain compatibility with the previous version. Includes unit tests for the... to Upgrade SimpleIpV4AddressValidator and SimpleIpV6AddressValidator.Sep 15 2018, 2:20 AM
andersonbruce edited the summary of this revision. (Show Details)

Thanks! And since I don't believe this patch on its own is sufficient to resolve 397572, please change FEATURE: 397572 to CCBUG: 397572. The former will close the bug once this lands; the latter just adds the commit info as a comment, but keeps the bug open--which is what we want here.

andersonbruce edited the summary of this revision. (Show Details)Sep 15 2018, 2:23 AM
pino requested changes to this revision.Sep 17 2018, 6:22 AM
pino added a dependent revision: D15093: Add WireGuard capability..
  • same notes for simpleipv6test.cpp as in simpleipv4test.cpp
tests/CMakeLists.txt
12–14

just use ecm_add_test instead

19–20

are these two libraries actually used?

27–29

ditto

34–35

ditto

tests/simpleipv4test.cpp
24

unused

44–46

even if this is a test, these are leaked, "polluting" the results of tools like valgrind; just make them non-pointers class variables

49

please use a data-driven approach for this file

54

QVERIFY(foo == bar) -> QCOMPARE(foo, bar), it applies to all the checks in this file

This revision now requires changes to proceed.Sep 17 2018, 6:23 AM
  • Merge new IP list validator into this branch
  • Update from review comments
andersonbruce marked 8 inline comments as done.Sep 18 2018, 5:03 AM
pino added a comment.Sep 18 2018, 5:17 AM
  • please remove all the empty initTestCase() in tests
libs/editor/simpleiplistvalidator.cpp
28–29

indentation

65

once again, simple char split with trim, please; you can use both splitRef + trimmed with QStringRef, so there is almost no waste of memory
using a regexp for a simple task like this is that it's way slower than using a simple character as separator

70

the variable i is only incremented, and never used

libs/editor/simpleipv4addressvalidator.cpp
100

while this change is OK, please do not mix it together with this patch

libs/editor/simpleipv6addressvalidator.cpp
48–50

unrelated changes

112

as above, unrelated change

tests/simpleipv4test.cpp
24

unused

tests/simpleipv6test.cpp
24

unused

  • Remove unnecessary includes and member functions
  • Fix a case that was being reported as Acceptable rather than Intermediate
andersonbruce marked 2 inline comments as done.Sep 25 2018, 9:44 AM
  • Fix review comments
andersonbruce marked 6 inline comments as done.Sep 25 2018, 10:34 AM
jgrulich accepted this revision.Oct 17 2018, 10:34 AM

Thanks for the contribution. I'll push this change and do just some minor changes to the tests so you don't have to again go through review.

CMakeLists.txt
99

Tests shouldn't be build automatically, you should add somethine like https://cgit.kde.org/networkmanager-qt.git/tree/CMakeLists.txt#n43.

This revision was not accepted when it landed; it landed in state Needs Review.Oct 17 2018, 10:37 AM
This revision was automatically updated to reflect the committed changes.