[Differential] [Requested Changes To] D4095: Refactor to embrace a cmake server import backend
Kevin Funk
noreply at phabricator.kde.org
Thu Jan 12 02:27:52 UTC 2017
kfunk requested changes to this revision.
kfunk added a reviewer: kfunk.
kfunk added a comment.
This revision now requires changes to proceed.
Just some quick review before I go to bed.
Some general remarks:
- Please use categorized logging everywhere
- Looots of lambdas, wouldn't mind getting rid off at least the ones with bigger "bodies"
INLINE COMMENTS
> cmakeserver.cpp:34
> +{
> + const QString path = QStandardPaths::writableLocation(QStandardPaths::TempLocation) + "/kdevelop-cmake-" + QString::number(KRandom::random());
> +
`QTemporaryFile`
> cmakeserver.cpp:76
> +
> +void CMakeServer::command(const QJsonObject& object)
> +{
Rename to `sendCommand`?
> cmakeserver.h:32
> + Q_OBJECT
> + public:
> + CMakeServer(QObject* parent);
Style: Deindent here & below
> cmakeserverimportjob.cpp:28
> +
> +template <typename T, typename Q, typename W>
> +static T kTransform(const Q& list, W func)
Style: Off in this whole file, try "Reformat source" with kdev_format script -- you'll be surprised - it works!
> cmakeserverimportjob.cpp:93
> + const auto configs = response.value("configurations").toArray();
> + qDebug() << "fuuuuuu" << response;
> + for (const auto &config: configs) {
How polite :)
> cmakeserverimportjob.h:34
> +{
> + Q_OBJECT
> + public:
Style: Deindent here & below
> test_cmakeserver.cpp:3
> + *
> + * Copyright 2006 Matt Rogers <mattr at kde.org>
> + *
He's the author? :)
> testhelpers.h:137
> qFatal( "Timeout while waiting for opened signal" );
> + Q_ASSERT(false);
> + }
Redundant (qFatal aborts already?)
REPOSITORY
R32 KDevelop
REVISION DETAIL
https://phabricator.kde.org/D4095
EMAIL PREFERENCES
https://phabricator.kde.org/settings/panel/emailpreferences/
To: apol, #kdevelop, kfunk
Cc: kfunk, kdevelop-devel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20170112/582c7bdc/attachment-0001.html>
More information about the KDevelop-devel
mailing list