Details

Reviewers

jgrulich
pino

Commits

R116:71833088fb26: Upgrade SimpleIpV4AddressValidator and SimpleIpV6AddressValidator

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

Harbormaster completed remote builds in B2873: Diff 41677.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

andersonbruce mentioned this in D15521: Add validator for lists of IP addresses Added as separate review per comment from Pino on review D15093. This code will not compile without the updated code in review D15520. Also includes unit test..Sep 16 2018, 5:19 AM

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

Harbormaster completed remote builds in B2980: Diff 41875.Sep 18 2018, 4:42 AM

andersonbruce marked 8 inline comments as done.Sep 18 2018, 5:03 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

Harbormaster completed remote builds in B2981: Diff 41878.Sep 18 2018, 5:35 AM

Fix a case that was being reported as Acceptable rather than Intermediate

Harbormaster completed remote builds in B3194: Diff 42281.Sep 25 2018, 6:49 AM

andersonbruce marked 2 inline comments as done.Sep 25 2018, 9:44 AM

Fix review comments

Harbormaster completed remote builds in B3199: Diff 42297.Sep 25 2018, 10:31 AM

andersonbruce marked 6 inline comments as done.Sep 25 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 ↗	(On Diff #41878)	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

Closed by commit R116:71833088fb26: Upgrade SimpleIpV4AddressValidator and SimpleIpV6AddressValidator (authored by andersonbruce, committed by jgrulich). · Explain Why

This revision was automatically updated to reflect the committed changes.

		Path
M		CMakeLists.txt (2 lines)
A	M	libs/editor/simpleiplistvalidator.cpp (108 lines)
M		libs/editor/simpleipv4addressvalidator.cpp (86 lines)
M		libs/editor/simpleipv6addressvalidator.cpp (135 lines)
A	M	tests/CMakeLists.txt (30 lines)
A	M	tests/simpleipv4test.cpp (152 lines)
A	M	tests/simpleipv6test.cpp (192 lines)

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	41677	7e578dd		Sep 15 2018, 2:18 AM	★	★
Diff 2	41875	7e578dd	- Merge new IP list validator into this branch	Sep 18 2018, 4:42 AM	★	★
Diff 3	41878	7e578dd	- Remove unnecessary includes and member functions	Sep 18 2018, 5:35 AM	★	★
Diff 4	42281	7e578dd	- Fix a case that was being reported as Acceptable rather than Intermediate	Sep 25 2018, 6:49 AM	★	★
Diff 5	42297	7e578dd	- Fix review comments	Sep 25 2018, 10:31 AM	★	★
Diff 6	43784	ea9a320	R116:71833088fb26be75ae88f13f970b814436b688ce	Oct 17 2018, 10:37 AM	★	★

Commit	Tree	Parents	Author	Summary	Date
ab8778e1284c	78a8eea945d7	de47d251b9b2	Druco	Fix a case that was being reported as Acceptable rather than Intermediate	Sep 25 2018, 6:48 AM
de47d251b9b2	54937cd62031	3a851fd6ff90	Druco	Remove unnecessary includes and member functions	Sep 18 2018, 5:34 AM
3a851fd6ff90	ed430614c923	14965d176688	Druco	Update from review comments	Sep 18 2018, 12:01 AM
14965d176688	a284f830e064	39fa05b3b01c	Druco	Merge new IP list validator into this branch	Sep 17 2018, 11:57 PM
39fa05b3b01c	c3e8b3c9c964	7e578ddc6873	Druco	Upgrade SimpleIpV4AddressValidator and SimpleIpV6AddressValidator to optionally… (Show More…)	Sep 15 2018, 2:07 AM

Upgrade SimpleIpV4AddressValidator and SimpleIpV6AddressValidator
ClosedPublic
Actions

Details

Diff Detail

Showing Only Differences

Revision Contents
Changeset List

Diff 42281

CMakeLists.txt

libs/editor/simpleiplistvalidator.cpp

libs/editor/simpleipv4addressvalidator.cpp

libs/editor/simpleipv6addressvalidator.cpp

tests/CMakeLists.txt

tests/simpleipv4test.cpp

tests/simpleipv6test.cpp

Upgrade SimpleIpV4AddressValidator and SimpleIpV6AddressValidatorClosedPublicActions

Details

Diff Detail

Showing Only Differences

Revision ContentsChangeset List

Diff 42281

CMakeLists.txt

libs/editor/simpleiplistvalidator.cpp

libs/editor/simpleipv4addressvalidator.cpp

libs/editor/simpleipv6addressvalidator.cpp

tests/CMakeLists.txt

tests/simpleipv4test.cpp

tests/simpleipv6test.cpp

Upgrade SimpleIpV4AddressValidator and SimpleIpV6AddressValidator
ClosedPublic
Actions

Revision Contents
Changeset List