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

[Milian Wolff]

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


  lgtm to me mostly, but some stuff still needs to be ironed out, imo

INLINE COMMENTS

> cmakemanager.cpp:143
>  
> +static bool serverSupported()
> +{

this is not used anywhere now?

> cmakemanager.cpp:167
>  
> -    // create the JSON file if it doesn't exist
> -    auto commandsFile = CMake::commandsFile(project);
> -    if (!QFileInfo::exists(commandsFile.toLocalFile())) {
> -        qCDebug(CMAKE) << "couldn't find commands file:" << commandsFile << "- now trying to reconfigure";
> -        jobs << builder()->configure(project);
> -    }
> +    CMakeServer* server = new CMakeServer(this);
> +    server->waitForConnected();

Who will delete the server when it is available? this looks like you are piling up servers until the CMakeManager is deleted at shutdown.

You probably want to use a unique_ptr here and transfer ownership into the import job when the server is available

> cmakeserver.cpp:63
> +        //Once the process has started, wait for the file to be created, then connect to it
> +        QTimer* connectTimer = new QTimer(this);
> +        connectTimer->setInterval(100);

you don't need the full timer object anymore, do you? Simply use QTimer::singleShot?

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

what if the first connect fails - we simply get an error and disconnect and don't try thereafter, right? couldn't we retry for up to N times (say every 100ms for up to 1s?) to make this a bit less brittle?

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

missing curly braces

> apol wrote in cmakeserver.cpp:45
>   apol at oliver:~$ cmake -E server --experimental --pipe=fu
>   ^C
>   apol at oliver:~$ cmake -E server --experimental --pipe fu
>   CMake Error: Unknown argument for server mode

:-S

> apol wrote in cmakeserver.cpp:128
> My brain wants to do it, but then it becomes an unreadable blob... :(

think of all the little allocations :P

> apol wrote in cmakeserver.h:29
> Well, whoever interacts with this is interacting with the server after all.

right, it's a client to the server, I'd welcome the rename personally

REPOSITORY
  R32 KDevelop

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

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

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