dhaumann added a comment.
The patch looks already much better. I have added comments below.
Btw, did you copy some code from another project? If so, we need to be
careful, since KTextEditor is LGPLv+2.
INLINE COMMENTS
> CMakeLists.txt:54
> +# EditorConfig support
> +# TODO: find oldest working version (0.12.0 was released 2014-10-12)
> +find_package(editorconfig "0.12.0")
I think you can just write "# EditorConfig support (0.12.0 was released
2014-10-12)"
There is no need for looking for even older versions I think.
> CMakeLists.txt:89
> document/katebuffer.cpp
> +document/editorconfig.cpp
>
Since the editorconfig is optional, I would make compiling this optional as
well:
if (EDITORCONFIG_FOUND) # optional support for EditorConfig
set(ktexteditor_LIB_SRCS
${ktexteditor_LIB_SRCS}
document/editorconfig.cpp
)
endif()
This way, it's truly optional :-)
> editorconfig.cpp:23
> +
> +EditorConfig::EditorConfig(KTextEditor::DocumentPrivate *document)
> +{
Better is to write:
EditorConfig::EditorConfig(KTextEditor::DocumentPrivate *document)
: m_document(document)
, m_handle(0)
{
// ...
}
> editorconfig.cpp:160
> +{
> + int code =
> editorconfig_parse(m_document->url().toLocalFile().toStdString().c_str(),
> m_handle);
> +
const int code = ...
Please use const *whenever possible* :-)
> editorconfig.h:56-57
> + static bool checkIntValue(QString value, int *result);
> + void interpret();
> + void interpretLine(const char *key, const char *value);
> +
I would prefer to merge these into parse(), then we also do not need all the
member variables anymore.
Rule of thumb: Each member variable adds more states to a class. The more
states, the less easy it is to understand when a member changes, since it can
be used in any function in the class. In contrast, if variables are local to a
function, this reduces states of the class. Therefore, it is typically much
easier to understand what the class is doing.
> katedocument.cpp:2589-2590
> + // file, if such is provided
> + EditorConfig *editorConfig = new EditorConfig(this);
> + editorConfig->parse();
> +#endif
No need to put this on the heap, especially since we are also missing a delete
here (memory leak). Just write:
EditorConfig editorConfig(this);
editorConfig.parse();
> katedocument.h:29-31
> +#ifdef EDITORCONFIG_FOUND
> +#include "editorconfig.h"
> +#endif
It is enough to move this to the .cpp file, since it is completely unused in
the header or by any other files.
REPOSITORY
R39 KTextEditor
REVISION DETAIL
https://phabricator.kde.org/D4537
EMAIL PREFERENCES
https://phabricator.kde.org/settings/panel/emailpreferences/
To: gszymaszek, #ktexteditor
Cc: dhaumann, kwrite-devel, #frameworks
