D4910: Initial version of Heaptrack analyzer plugin

[Kevin Funk]

kfunk requested changes to this revision.
kfunk added a comment.
This revision now requires changes to proceed.


  Nice addition!

INLINE COMMENTS

> CMakeLists.txt:4
> +include_directories(
> +    ${CMAKE_CURRENT_BINARY_DIR}
> +)

Shouldn't be needed. Done via:

  kde-modules/KDECMakeSettings.cmake
  189:   set(CMAKE_INCLUDE_CURRENT_DIR ON)

> globalconfigpage.h:33
> +    GlobalConfigPage(KDevelop::IPlugin* plugin, QWidget* parent);
> +    virtual ~GlobalConfigPage() = default;
> +

`override`

> debug.cpp:22
> +
> +Q_LOGGING_CATEGORY(KDEV_HEAPTRACK, "kdev.heaptrack")

Please use `"kdevelop.analyzers.heaptrack"` instead (so we all share the same category prefix).

If possible please update the other analyzers as well.

> debug.h:24
> +
> +Q_DECLARE_LOGGING_CATEGORY(KDEV_VERAPP)

`KDEV_VERAPP` -> `KDEV_HEAPTACK`

> job.cpp:155
> +
> +    switch (e) {
> +    case QProcess::FailedToStart:

This whole casing on `e` is already handled by `OutputExecuteJob::childProcessError` already, no? No need to duplicate code.

> plugin.cpp:120
> +
> +    auto heaptrackJob = new Job(dlg.pidSelected());
> +    connect(heaptrackJob, &Job::finished, this, &Plugin::jobFinished);

This truncates a `long int` into an `int` (potentially losing information under Windows 64 bit).

Use a `long int` for `Job::m_pid`

> plugin.cpp:135
> +
> +    qDebug() << job->status();
> +    if (job->status() == KDevelop::OutputExecuteJob::JobStatus::JobSucceeded) {

`qCDebug`

> utils.h:28
> +{
> +    QString findExecutable(const QString &executableName);
> +

`executableName` -> `fallbackExecutableName`?

> visualizer.cpp:38
> +{
> +    connect(this, &QProcess::errorOccurred, this, [this]() {
> +        QString errorMessage;

Could capture and use `error` from the signal here.

> visualizer.cpp:63
> +{
> +//     disconnect();
> +    QFile::remove(m_resultsFile);

Remove commented code

> visualizer.h:32
> +public:
> +    Visualizer(const QString& resultsFile, QObject* parent);
> +    ~Visualizer() override;

Should probably add some documentation that `Visualizer` does delete `resultFile` on destruction.

REPOSITORY
  R32 KDevelop

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

To: antonanikin, mwolff, kfunk
Cc: kfunk, kdevelop-devel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20170307/274da90b/attachment-0001.html>