CCBUG: 397572
FIXED-IN: 5.14.0
Diff Detail
Diff Detail
- Repository
- R116 Plasma Network Management Applet
- Branch
- IPAddressListValidator
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 2912 Build 2930: arc lint + arc unit
| libs/editor/CMakeLists.txt | ||
|---|---|---|
| 92 | I'm sure you don't need to add all these and why did you remove PUBLIC and PRIVATE keywords? | |
| 111 | Why removing it? | |
| 115 | This is also an unrelated change. | |
| libs/editor/simpleiplistvalidator.cpp | ||
| 27 | Fix indentation. | |
| 39 | In both validator constructors (mean also for IPv6 one), you can directly pass "style" param from contructor, given both enums seem to be identical. | |
| tests/CMakeLists.txt | ||
| 9 | Do we need these flags to be set? Please leak how simply can tested be added to CMake [1] - https://cgit.kde.org/networkmanager-qt.git/tree/autotests/CMakeLists.txt | |
Comment Actions
- same note as in D15520: Upgrade SimpleIpV4AddressValidator and SimpleIpV6AddressValidator 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
| libs/editor/simpleiplistvalidator.cpp | ||
|---|---|---|
| 29 | please set both m_ipv4Validator and m_ipv6Validator, otherwise they can be left uninitialized when type is not Both | |
| 62 | no need for a regexp, please use QString::split() with QString::SkipEmptyParts | |
| 67 | set (later on), but never use | |
| 101 | result will never be QValidator::Invalid, so this condition is always true | |
| tests/CMakeLists.txt | ||
| 13–15 | just use ecm_add_test instead | |
| 26–28 | ditto | |
| 42–44 | ditto | |
| tests/simpleiplisttest.cpp | ||
| 23 | unused | |
| 43–45 | even if this is a test, these are leaked, "polluting" the results of tools like valgrind; just make them non-pointers class variables | |
| 53 | QVERIFY(foo == bar) -> QCOMPARE(foo, bar), it applies to all the checks in this file | |
| libs/editor/CMakeLists.txt | ||
|---|---|---|
| 92 | Damn, copied the wrong file. Sorry about that. | |
Comment Actions
Can you please discuss this with Pino, he was the one who suggested separate reviews.
I will follow whatever you decide but if you decide that they should be merged please let me know how to do this.
| libs/editor/simpleiplistvalidator.cpp | ||
|---|---|---|
| 39 | 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. | |
| 62 | 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. | |
| 67 | 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. | |
| tests/simpleiplisttest.cpp | ||
| 53 | Any particular reason you think this is superior? | |