D14288: Initial version of Clazy analyzer plugin

[Friedrich W. H. Kossebau]

kossebau added a comment.


  Sadly too tired right now to grasp the whole codebase for a decided last commnet. And not an expert in KDevelop API for analysis tools :)
  But from what I tested today with latest codebase, and what I understood from the code, this should be good to be merged (to 5.3 branch).
  
  So my +1 for merging on Wednesday, if no-one else has further objections (will see to review tomorrow morning again with fresh mind :) ).

INLINE COMMENTS

> checksdb.cpp:79
> +    for (const auto& levelDir : levelsDirs) {
> +        static const QRegularExpression levelRE(QStringLiteral(".*level.*"));
> +

ChecksDB constructor is not too often called,so no need to cache this in a static. instead move just out of the loop.

> checksdb.cpp:97
> +        for (const auto& checkFile : checksFiles) {
> +            static const QRegularExpression checkRE(QStringLiteral("^README-(.+)\\.md$"));
> +

same, move out of loop instead

> checkswidget.cpp:148
> +    const QString& levelName,
> +    const QList<const QTreeWidgetItem*> levelItems)
> +{

& missing for const reference type

> checkswidget.cpp:206
> +
> +    auto checks = checksList.join(',');
> +    if (m_checks != checks) {

If possible, prepare already for no-implicit ascii->QString/QChar conversion, using QLatin1Char here and respective elsewhere.

> antonanikin wrote in checkswidget.cpp:134
> Ui::ChecksWidget is incomplete type for default destructor.

Only if you use that in the class declaration, right? Actually meant this could be done here, like this:

  ChecksWidget::~ChecksWidget() = default;

> checkswidget.h:56
> +Q_SIGNALS:
> +    void checksChanged(const QString& cheks);
> +

typo che_c_ks

> checkswidget.ui:74
> +     <property name="text">
> +      <string>Since nothing is selected Clazy will use all checks from levels 0 and 1</string>
> +     </property>

Finish sentence with a "."

> job.cpp:44
> +
> +// Empty constructor which creates invalid Job instance. Used only for testing
> +Job::Job()

This comment might be better as api dox on the method in the class declaration.

> job.cpp:97
> +
> +    scriptStream << QStringLiteral("SOURCES = %1\n").arg(clazySources.join(' '));
> +

Hm, possibly instead of putting all files into a single line, adding them line by line might be better, with many files and deep project paths?

Also might be easier for the code reader if the escaping is done here, it comes a little by surprise that sources() already delivered the escaped variants.

> jobparameters.cpp:112
> +{
> +    const auto dataPaths = QStandardPaths::standardLocations(QStandardPaths::GenericDataLocation);
> +    for (const auto& dataPath : dataPaths)

Why not using QStandardPaths::locate()?

> jobparameters.h:30
> +{
> +
> +class IProject;

empty lines before/after could be removed, following pattern elsewher:

  namespace KDevelop
  {
  classIProject;
  }

> plugin.cpp:62
> +    reloadDB();
> +
> +    const QIcon clazyIcon = QIcon::fromTheme(QStringLiteral("clazy"));

Ideally the loading of the db could be delayed until needed, given this is in the hotpath of starting.

> antonanikin wrote in plugin.cpp:251
> We may want to check generated files, for example. Or you think we should disable such checking?

Hm, unsure. Given we cannot influence the generating tools, such checks might not help that much. But perhaps some people still prefer to be able to do that, so perhaps we can keep it, does not hurt anyone I guess :)

> problemmodel.cpp:81
> +    m_problems.append(problems);
> +    for (auto p : problems) {
> +        addProblem(p);

`auto& o`, no need to create another non-trivial pointer object copy.

> utils.cpp:70
> +
> +    static const QString KEY_FILE = QStringLiteral("file");
> +

static is not really needed here, there is not anything special about this over the other QStringLiterals?

REPOSITORY
  R32 KDevelop

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

To: antonanikin, #kdevelop
Cc: mwolff, apol, kfunk, brauch, 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/20180820/86f745ab/attachment-0001.html>