[Differential] [Requested Changes To] D4095: Refactor to embrace a cmake server import backend

[Milian Wolff]

mwolff requested changes to this revision.
mwolff added a reviewer: mwolff.
mwolff added a comment.
This revision now requires changes to proceed.


  awesome work Aleix! I got a few nitpicks, but in general this looks really good already. I'd say clean it up a bit and then merge it?
  
  The only things that may require some more discussion are the startup procedure for the server instead of relying on the 100ms delay, and whether the "serverSupported" flag is global or per-project (i.e. per-cmake binary)

INLINE COMMENTS

> cmakemanager.cpp:142
>  
> +Q_GLOBAL_STATIC_WITH_ARGS(bool, s_serverSupported, (false))
> +

for PODs, you don't need Q_GLOBAL_STATIC. simply make this a

namespace {
static bool s_serverSupported = false;
}

on a conceptual level - this depends on the cmake binary that can be selected for every project, no? So this should not be a static at all, but rather a per-project property?

> cmakemanager.cpp:154
> +            if (job->error() != 0) {
> +                qCDebug(CMAKE) << "couldn't load successfully" << job->project()->name();
> +                m_projects.remove(job->project());

make this a warning? also, do we want to show the user that? this can be serious and render the whole cmake support useless after all, right?

> cmakemanager.cpp:174
> +            if (job->error() != 0) {
> +                qCDebug(CMAKE) << "couldn't load successfully" << job->project()->name();
> +                m_projects.remove(job->project());

warning, and see above? actually the whole code is duplicated - move this into a proper slot? or at least share the lambda body?

> cmakeprojectdata.cpp:3
> + *
> + * Copyright 2013 Aleix Pol <aleixpol at kde.org>
> + *

the future is now! it's 2017 :P

> cmakeprojectdata.cpp:31
> +    , targets(targets)
> +    , watcher(new QFileSystemWatcher)
> +    , m_testSuites(tests)

missing parent?

> cmakeprojectdata.cpp:33
> +    , m_testSuites(tests)
> +{}

empty line at end please

> cmakeprojectdata.h:61
>  {
> +    CMakeProjectData(const QHash<KDevelop::Path, QStringList>& targets, const CMakeFilesCompilationData &data, const QVector<Test> &tests);
> +

& next to type names

> cmakeprojectdata.h:71
> +
> +    QVector<Test> testSuites() const { return m_testSuites; }
> +

simply make it public like the stuff above and remove the getter?

> cmakeserver.cpp:3
> + *
> + * Copyright 2016 Aleix Pol <aleixpol at kde.org>
> + *

2017

> cmakeserver.cpp:24
> +#include "cmakeutils.h"
> +#include <KRandom>
> +#include <QJsonDocument>

unused?

> cmakeserver.cpp:38
> +    {
> +        QTemporaryFile file("kdevelopcmake-");
> +        file.open();

keep it around, otherwise the file gets removed again, no? or is that OK?

> cmakeserver.cpp:45
> +    m_process.setProcessChannelMode(QProcess::ForwardedChannels);
> +    m_process.start(CMake::findExecutable(), {"-E", "server", "--experimental", "--pipe=" + path});
> +

QStringLiteral, and I hope cmake supports "--pipe", "path" as two args? i.e. instead of `--pipe=path`, can you pass `--pipe path`?

> cmakeserver.cpp:62
> +    QTimer* connectTimer = new QTimer(this);
> +    connectTimer->setInterval(100);
> +    connectTimer->setSingleShot(true);

can you add a comment saying why this is required? is there really no "good" way to wait for the server to start up? what is creator doing here? have you talked to thunger about this? looks really brittle

> cmakeserver.cpp:67
> +    });
> +    connect(&m_process, &QProcess::started, connectTimer, static_cast<void(QTimer::*)()>(&QTimer::start));
> +}

I'd simplify the above a bit (imo) by doing something like:

  connect(...started, this, [this, path] () {
              // please document me
              QTimer::singleShot(100, this, [this, path] () {
                  m_localSocket->connectToServer(path, QIODevice::ReadWrite);
              });
          });

> cmakeserver.cpp:81
> +
> +static const QByteArray openTag  = "\n[== \"CMake Server\" ==[\n";
> +static const QByteArray closeTag = "\n]== \"CMake Server\" ==]\n";

QByteArrayLiteral, wrap in functions returning QByteArray to get rid of global static initialization

> cmakeserver.cpp:90
> +    auto len = m_localSocket->write(data);
> +    QTextStream(stdout) << "writing...\n" << QJsonDocument(object).toJson();
> +    Q_ASSERT(len>0);

qCDebug(...) << "writing..." << object;

> cmakeserver.cpp:91
> +    QTextStream(stdout) << "writing...\n" << QJsonDocument(object).toJson();
> +    Q_ASSERT(len>0);
> +}

spaces around operators

> cmakeserver.cpp:98
> +
> +    m_buffer += m_localSocket->readAll();
> +    for(; m_buffer.size() > openTag.size(); ) {

const auto openTag = ::openTag(); // once you added the getter
  const auto oepnTagSize = openTag.size();
  // use both below

> cmakeserver.cpp:99
> +    m_buffer += m_localSocket->readAll();
> +    for(; m_buffer.size() > openTag.size(); ) {
> +

this can go into an infinite-loop, no?

if m_buffer.size() == openTag.size() + 1, then we go into the loop, but we won't find the close tag and thus cannot ever get out of the loop

see below

> cmakeserver.cpp:103
> +        const int idx = m_buffer.indexOf(closeTag, openTag.size());
> +        if (idx>=0) {
> +            emitResponse(m_buffer.mid(openTag.size(), idx - openTag.size()));

spaces around operators

> cmakeserver.cpp:106
> +            m_buffer = m_buffer.mid(idx + closeTag.size());
> +        }
> +    }

else break?

> cmakeserver.cpp:117
> +    }
> +    Q_ASSERT(!error.error);
> +    Q_ASSERT(doc.isObject());

since we don't generate the response data, I wouldn't assert on it. if there's a cmake bug it would kill kdevelop...

> cmakeserver.cpp:128
> +    sendCommand({
> +        {"cookie", ""},
> +        {"type", "handshake"},

here and below: QStringLiteral

> cmakeserver.h:3
> + *
> + * Copyright 2016 Aleix Pol <aleixpol at kde.org>
> + *

dito, 2017

> cmakeserver.h:29
> +
> +class KDEVCMAKECOMMON_EXPORT CMakeServer : public QObject
> +{

this is actually a CMakeServerClient, right?

> cmakeserverimportjob.cpp:3
> + *
> + * Copyright 2014 Kevin Funk <kfunk at kde.org>
> + *

really?

> cmakeserverimportjob.cpp:30
> +template <typename T, typename Q, typename W>
> +static T kTransform(const Q& list, W func)
> +{

you only use it with T == Path::List, so why not hardcode that?

> cmakeserverimportjob.cpp:39
> +
> +///copied from the makefile resolver
> +static QRegularExpression defineRegularExpression()

please add it as a getter to the MakefileResolver API, which is used elsewhere in CMake already. That way we don't duplicate this magic

> cmakeserverimportjob.cpp:43
> +  static const QRegularExpression pattern(
> +    "-D([^\\s=]+)(?:=(?:\"(.*?)(?<!\\\\)\"|([^\\s]*)))?"
> +  );

QStringLiteral

> cmakeserverimportjob.cpp:69
> +    QHash<QString, QString> ret;
> +    const auto& defineRx = defineRegularExpression();
> +    auto it = defineRx.globalMatch(compileFlags);

this could probably be part of a helper function in the MakefileResolver (see above), since that does something like that already elsewhere, I guess?

> cmakeserverimportjob.cpp:83
> +        const int eqIdx = define.indexOf(QLatin1Char('='));
> +        if (eqIdx<0)
> +            ret[define] = QString();

spaces around operators

> cmakeserverimportjob.cpp:146
> +    connect(m_server, &CMakeServer::response, this, [this](const QJsonObject &response) {
> +        if (response.value("type") == QLatin1String("reply")) {
> +            const auto inReplyTo = response.value("inReplyTo");

QStringLiteral for the "type"

> cmakeserverimportjob.cpp:146
> +    connect(m_server, &CMakeServer::response, this, [this](const QJsonObject &response) {
> +        if (response.value("type") == QLatin1String("reply")) {
> +            const auto inReplyTo = response.value("inReplyTo");

introduce var for type, as it's used again below

> cmakeserverimportjob.cpp:147
> +        if (response.value("type") == QLatin1String("reply")) {
> +            const auto inReplyTo = response.value("inReplyTo");
> +            qCDebug(CMAKE) << "replying..." << inReplyTo;

QStringLiteral

> cmakeserverimportjob.cpp:149
> +            qCDebug(CMAKE) << "replying..." << inReplyTo;
> +            if (inReplyTo == "handshake")
> +                m_server->configure({});

QLatin1String

> cmakeserverimportjob.cpp:161
> +        } else if(response.value("type") == QLatin1String("error")) {
> +            setError(2);
> +            setErrorText(response.value("errorMessage").toString());

what's this magic number? is there no enum for this?

> cmakeserverimportjob.h:2-3
> +/* KDevelop CMake Support
> + *
> + * Copyright 2014 Kevin Funk <kfunk at kde.org>
> + *

really?

> cmakeserverimportjob.h:34
> +{
> +Q_OBJECT
> +public:

indent

> test_cmakemanager.cpp:283
>  
> +    QEXPECT_FAIL("", "Will fix soon, hopefully", Abort);
>      ProjectTargetItem *target = targets.first();

famous last words :D

> test_cmakeserver.cpp:3
> + *
> + * Copyright 2016 Aleix Pol Gonzalez <aleixpol at kde.org>
> + *

2017

REPOSITORY
  R32 KDevelop

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

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: apol, kfunk, #kdevelop, mwolff
Cc: mwolff, antonanikin, arrowdodger, kfunk, kdevelop-devel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20170122/12156044/attachment-0001.html>