D14288: Initial version of Clazy analyzer plugin

[Friedrich W. H. Kossebau]

kossebau added a comment.


  Here some nitpicks while scanning over the updated code. In general looks very good to me :)
  
  All the lambdas are challenging my pre-C++11 code trained reading brain :) so have some personal big question mark on them, if that is better style. Not an expert here.
  
  Going now to test-drive the plugin a bit, will report later on the experience.

INLINE COMMENTS

> checksdb.cpp:77
> +
> +    auto levelsDirs = docsDir.entryList(QDir::Dirs | QDir::NoDotAndDotDot);
> +    for (const auto& levelDir : levelsDirs) {

const, please (also good for the range-based foor loop below)

> checksdb.cpp:95
> +
> +        auto checksFiles = docsDir.entryList(QDir::Files | QDir::Readable);
> +        for (const auto& checkFile : checksFiles) {

more const :)

> checkswidget.cpp:53
> +
> +    connect(m_ui->filterEdit, &KTreeWidgetSearchLine::searchUpdated, this, [this](const QString& searchString) {
> +        if (searchString.isEmpty()) {

Curious: why a lambda instead of a member method? What are the adavantages, what the disadvantages?

Possibly a matter of taste and what the code-reading brain is trained to, but I find such long lambdas hard to read right now, and also am not used to this in the code  I have seen, so happy to extend my worldview :)

> checkswidget.cpp:134
> +
> +ChecksWidget::~ChecksWidget()
> +{

= default

> commandlinewidget.cpp:41
> +
> +CommandLineWidget::~CommandLineWidget()
> +{

= default

> projectconfigpage.cpp:126
> +
> +ProjectConfigPage::~ProjectConfigPage()
> +{

= default

> jobparameters.h:69
> +public:
> +    JobParameters(KDevelop::IProject* project);
> +    JobParameters(KDevelop::IProject* project, const QString& checkPath);

explicit

> plugin.cpp:66
> +    m_menuActionFile = new QAction(clazyIcon, i18n("Analyze Current File with Clazy"), this);
> +    connect(m_menuActionFile, &QAction::triggered, this, [this](){
> +        runClazy(false);

IMHO the lambdas could be deduplicated (or perhaps turned into normal class methods, whatever is the better approach).

> plugin.cpp:111
> +{
> +    return m_job;
> +}

Other than in if-statements here IMHO an explicit `!= nullptr` makes the code better to read on quick parsing.

> plugin.cpp:226
> +
> +    m_job = nullptr; // job is automatically deleted later
> +

be more explicit and ssay who takes care of that: "job automatically deletes itself later", so the code reader does not potentially wrongly expect the plugin code to do it elsewhere.

> plugin.cpp:251
> +            case KDevelop::ProjectBaseItem::Folder:
> +            case KDevelop::ProjectBaseItem::BuildFolder:
> +                break;

Build folder as well?

> plugin.h:38
> +
> +class Plugin : public KDevelop::IPlugin
> +{

Please add here or perhaps in a separate README file in the directory  a short description about the approach the plugin takes to make use of clazy, .i.e. the generation of a Makefile which then is used with make and clazy.

> test_clazyjob.cpp:29
> +
> +#include <QtTest/QTest>
> +

Please remove module prefix QtTest

> antonanikin wrote in test_clazyjob.cpp:82
> Clazy is runtime dependency, so I think we should always run the test.

A failing test though means something is broken in the code/logic. FAIL sends a wrong message here IMHO.
+1 for QSKIP if runtime dep is not found.

> utils.cpp:116
> +
> +    ~MarkdownConverter()
> +    {

= default

> utils.cpp:223
> +        if (state != PREFORMATTED) {
> +            line.replace(QLatin1String("&"), QLatin1String("&"));
> +            line.replace(QLatin1String("<"), QLatin1String("<"));

mini optimization: use QLatin1Char for single letter, using the `replace(QChar, QLatin1String) ` overload

> utils.cpp:238
> +
> +    int state;
> +    QVector<QString> tagStart;

split off data member with oen `private:` section

REPOSITORY
  R32 KDevelop

REVISION DETAIL
  https://phabricator.kde.org/D14288

To: antonanikin, #kdevelop
Cc: pino, kossebau, kdevelop-devel, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight, arrowd
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20180817/b036994a/attachment-0001.html>