dhaumann added a comment.
Patch looks already pretty good, I think we're soon there. INLINE COMMENTS > gszymaszek wrote in editorconfig.cpp:23 > Is it OK to initialize `m_handle` in the constructor? If so, is `m_handle(0)` > necessary? Yes, this is good now. > editorconfig.cpp:34-56 > +bool EditorConfig::checkBoolValue(QString val, bool *result) > +{ > + val = val.trimmed().toLower(); > + static const QStringList trueValues = QStringList() << > QStringLiteral("1") << QStringLiteral("on") << QStringLiteral("true"); > + if (trueValues.contains(val)) { > + *result = true; > + return true; Can you move this into an unnamed namespace at the beginning of the file? Then, you can move the API documentation from the header file to the cpp file as well (and remove the static functions from the private section). > editorconfig.cpp:80-82 > + const char *key, *value; > + key = nullptr; > + value = nullptr; Please only one variable per line: const char *key = nullptr; const char *value = nullptr; > editorconfig.cpp:84-85 > + > + // their Qt counterparts, for comparisons > + QLatin1String keyString, valueString; > + Please declare variables locally, i.e. move down, see below. > editorconfig.cpp:105-106 > + > + keyString = QLatin1String(key); > + valueString = QLatin1String(value); > + const QLatin1String keyString = ... const QLatin1String valueString = ... REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D4537 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: gszymaszek, #ktexteditor Cc: cullmann, dhaumann, kwrite-devel, #frameworks