[Differential] [Updated] D4095: Refactor to embrace a cmake server import backend

Tue Jan 24 01:33:29 UTC 2017

apol marked 27 inline comments as done.
apol added a comment.

  That was one thorough review! Thanks a lot milian!!

  Will be submitting a new version with the fixes. Then if there's no big issue I'll merge to master by the end of the week, so we can start testing.

INLINE COMMENTS

> mwolff wrote in cmakemanager.cpp:174
> warning, and see above? actually the whole code is duplicated - move this into a proper slot? or at least share the lambda body?

As is we can't, they should inherit from the same class. I don't think it's a big deal at the moment...

> mwolff wrote in cmakeprojectdata.cpp:31
> missing parent?

Hm? It's not a QObject.

> mwolff wrote in cmakeserver.cpp:38
> keep it around, otherwise the file gets removed again, no? or is that OK?

No, actually we don't want the file at all. We just want a temporary and unique path but no file.

> mwolff wrote in cmakeserver.cpp:45
> QStringLiteral, and I hope cmake supports "--pipe", "path" as two args? i.e. instead of `--pipe=path`, can you pass `--pipe path`?

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

> mwolff wrote in cmakeserver.cpp:62
> 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

Could be a good idea to talk with him about this, yes.

> mwolff wrote in cmakeserver.cpp:106
> else break?

Yep, actually I added it yesterday when testing. Good catch! :D

> mwolff wrote in cmakeserver.cpp:117
> since we don't generate the response data, I wouldn't assert on it. if there's a cmake bug it would kill kdevelop...

Yeah... I added it when cmake was crashing. I'll remove for now.

> mwolff wrote in cmakeserver.cpp:128
> here and below: QStringLiteral

My brain wants to do it, but then it becomes an unreadable blob... :(

> mwolff wrote in cmakeserver.h:3
> dito, 2017

xD well I wrote this code in 2016.

Ah, time, thou art a heartless bitch,

> mwolff wrote in cmakeserver.h:29
> this is actually a CMakeServerClient, right?

Well, whoever interacts with this is interacting with the server after all.

> mwolff wrote in test_cmakemanager.cpp:283
> famous last words :D

Sh... at least we are testing a bit more than we used to...

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/20170124/eb8301f7/attachment-0001.html>