[Differential] [Commented On] D4537: EditorConfig support

[Dominik Haumann]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20170215/a538b35f/attachment-0001.html>