pino requested changes to this revision. pino added a comment. This revision now requires changes to proceed.
In D15093#324457 <https://phabricator.kde.org/D15093#324457>, @andersonbruce wrote: > A couple of questions on opening other review requests: > > - Should I open a bug request on bugs.kde.org before opening the review? No need to, they are only internal refactorings/improvements. > - Is there any special way I need to mark dependencies? That is, the simpleiplistvalidator is dependent on the updates to the simpleipv4/6 validators and WireGuard is dependent on both. I think so: see on the top-right "Edit Related Revisions" -> "Edit Parent/Child Revisions". > - Is there a preferred unit test framework that KDE uses? Qt ships its own QtTest -- see http://doc.qt.io/qt-5/qtest-overview.html > And do you possibly have a pointer to a sample of other projects that use it? One simple example is in juk, see the `tests` subdirectory. INLINE COMMENTS > simpleiplistvalidator.cpp:27-28 > +SimpleIpListValidator::SimpleIpListValidator(QObject *parent, > + enum AddressStyle > style, > + enum AddressType type) > + : QValidator(parent) this is not C, so 'enum' is not needed > simpleiplistvalidator.cpp:31-34 > + addressStyle = style; > + addressType = type; > + ipv4Validator = nullptr; > + ipv6Validator = nullptr; these can be moved to the constructor initialization list, so it's slightly more efficient > simpleiplistvalidator.cpp:42 > + ipv4Style = SimpleIpV4AddressValidator::AddressStyle::WithCidr; > + ipv4Validator = new SimpleIpV4AddressValidator(nullptr, ipv4Style); > + } this is leaked -- either you pass 'this' as parent, or explicitly delete them in the class destructor > simpleiplistvalidator.cpp:50 > + ipv6Style = SimpleIpV6AddressValidator::AddressStyle::WithCidr; > + ipv6Validator = new SimpleIpV6AddressValidator(nullptr, ipv6Style); > + } ditto > simpleiplistvalidator.cpp:61 > + // Split the incoming address on commas possibly with spaces on either > side > + QStringList addressList = address.split(QRegExp("\\s*,\\s*")); > + a regexp is not needed here, just pass QString::SkipEmptyParts to split() > simpleiplistvalidator.cpp:67-68 > + int i = 0; > + QValidator::State ipv4Result = QValidator::Acceptable; > + QValidator::State ipv6Result = QValidator::Acceptable; > + QValidator::State result = QValidator::Acceptable; these can be moved inside the foreach loop > simpleiplistvalidator.cpp:78 > + if (ipv4Validator != nullptr) > + ipv4Result = ipv4Validator->validate(const_cast<QString&>(addr), > localPos); > + else this const_cast is not nice... rather keep a local copy of the string > simpleiplistvalidator.cpp:85 > + if (ipv6Validator != nullptr) > + ipv6Result = ipv6Validator->validate(const_cast<QString&>(addr), > localPos); > + else ditto > simpleiplistvalidator.cpp:91-92 > + if (ipv6Result == QValidator::Invalid && ipv4Result == > QValidator::Invalid) { > + result = QValidator::Invalid; > + break; > + } i think you can just `return QValidator::Invalid` straight away > simpleiplistvalidator.h:42-45 > + AddressStyle addressStyle; > + AddressType addressType; > + SimpleIpV6AddressValidator *ipv6Validator; > + SimpleIpV4AddressValidator *ipv4Validator; private class variables start with "m_" prefix > simpleipv4addressvalidator.cpp:26 > > -SimpleIpV4AddressValidator::SimpleIpV4AddressValidator(QObject *parent) > +SimpleIpV4AddressValidator::SimpleIpV4AddressValidator(QObject *parent, enum > AddressStyle style) > : QValidator(parent) as above, no "enum" > simpleipv4addressvalidator.cpp:29 > { > + addressStyle = style; > } in constructor initialization list > simpleipv4addressvalidator.cpp:55-66 > + v = new QRegExpValidator(QRegExp(QLatin1String("[0-9, ]{1,3}\\.[0-9, > ]{1,3}\\.[0-9, ]{1,3}\\.[0-9, ]{1,3}")), 0); > + break; > + > + case WithCidr: > + v = new QRegExpValidator(QRegExp(QLatin1String("([0-9, > ]{1,3}\\.){3,3}[0-9, ]{1,3}/[0-9]{1,2}")), 0); > + break; > + all the regexps and their validators are created every time, and this is going to be super slow; since the type is decided at constructor time and never changed, just create the validator once (alos, this code leaks all the validators) > simpleipv4addressvalidator.cpp:120 > > + > // replace input string with the corrected version unneeded empty line change > simpleipv4addressvalidator.cpp:137 > + } else { > + value += QString::number(cidrValue); > + return QValidator::Acceptable; there is already `cidrParts[1]` as string, so no need to create it again from the value > simpleipv4addressvalidator.cpp:148 > + } else { > + value += QString::number(portValue); > + return QValidator::Acceptable; ditto > simpleipv4addressvalidator.h:44 > +private: > + AddressStyle addressStyle; > }; "m_" prefix > simpleipv6addressvalidator.cpp:26 > > -SimpleIpV6AddressValidator::SimpleIpV6AddressValidator(QObject *parent) > +SimpleIpV6AddressValidator::SimpleIpV6AddressValidator(QObject *parent, enum > AddressStyle style) > : QValidator(parent) no need for "enum" > simpleipv6addressvalidator.cpp:29 > { > + addressStyle = style; > } in constructor initialization list > simpleipv6addressvalidator.cpp:50-57 > + case Base: > + v = new > QRegExpValidator(QRegExp(QLatin1String("([0-9a-fA-F]{1,4}|:)+")), nullptr); > + break; > + > + case WithCidr: > + v = new > QRegExpValidator(QRegExp(QLatin1String("([0-9a-fA-F]{1,4}|:)+/[0-9]{1,3}")), > nullptr); > + break; as above, created every time, and leaked > simpleipv6addressvalidator.h:44 > +private: > + AddressStyle addressStyle; > }; "m_" prefix REPOSITORY R116 Plasma Network Management Applet REVISION DETAIL https://phabricator.kde.org/D15093 To: andersonbruce, #plasma, jgrulich, pino Cc: acrouthamel, K900, pino, lbeltrame, ngraham, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
