Code cleanup and tweaking (partially driven by clang-tidy)

Mainly it's about const-ification (in particular, passing const-refs instead of values) and deleting unneeded declarations/#includes. Since the changes alter the external interface, this is submitted as a PR for peer review.

One of unneeded declarations/definitions is a virtual destructor in BaseJob descendants. Since a job object should be deleted through QObject::deleteLater() anyway (and it's the only correct way of disposing of the object), all deletions will call the stack of destructors through virtual QObject::~QObject(). Therefore even BaseJob could get on with a non-virtual destructor but for the sake of clarity BaseJob::~BaseJob() is still declared virtual.

22 files changed, 74 insertions, 114 deletions

diff --git a/jobs/basejob.cpp b/jobs/basejob.cpp
index 3e2ecd54..f5de75f4 100644
--- a/jobs/basejob.cpp
+++ b/jobs/basejob.cpp
@@ -24,7 +24,6 @@
 #include <QtNetwork/QNetworkAccessManager>
 #include <QtNetwork/QNetworkRequest>
 #include <QtNetwork/QNetworkReply>
-#include <QtNetwork/QSslError>
 #include <QtCore/QTimer>
 
 #include "../connectiondata.h"
@@ -44,16 +43,18 @@ struct NetworkReplyDeleter : public QScopedPointerDeleteLater
 class BaseJob::Private
 {
     public:
-        Private(ConnectionData* c, HttpVerb v, QString endpoint,
-                const QUrlQuery& q, const Data& data, bool nt)
-            : connection(c), verb(v), apiEndpoint(endpoint), requestQuery(q)
-            , requestData(data), needsToken(nt)
-            , reply(nullptr), status(NoError)
+        // Using an idiom from clang-tidy:
+        // http://clang.llvm.org/extra/clang-tidy/checks/modernize-pass-by-value.html
+        Private(const ConnectionData* c, HttpVerb v,
+                QString endpoint, QUrlQuery q, Data data, bool nt)
+            : connection(c), verb(v), apiEndpoint(std::move(endpoint))
+            , requestQuery(std::move(q)), requestData(std::move(data))
+            , needsToken(nt), reply(nullptr), status(NoError)
         { }
         
         inline void sendRequest();
 
-        ConnectionData* connection;
+        const ConnectionData* connection;
 
         // Contents for the network request
         HttpVerb verb;
@@ -77,9 +78,9 @@ inline QDebug operator<<(QDebug dbg, const BaseJob* j)
     return dbg << "Job" << j->objectName();
 }
 
-BaseJob::BaseJob(ConnectionData* connection, HttpVerb verb, QString name,
-                 QString endpoint, BaseJob::Query query, BaseJob::Data data,
-                 bool needsToken)
+BaseJob::BaseJob(const ConnectionData* connection, HttpVerb verb,
+                 const QString& name, const QString& endpoint,
+                 const Query& query, const Data& data, bool needsToken)
     : d(new Private(connection, verb, endpoint, query, data, needsToken))
 {
     setObjectName(name);
@@ -96,12 +97,12 @@ BaseJob::~BaseJob()
     qCDebug(JOBS) << this << "destroyed";
 }
 
-ConnectionData* BaseJob::connection() const
+const ConnectionData* BaseJob::connection() const
 {
     return d->connection;
 }
 
-const QUrlQuery&BaseJob::query() const
+const QUrlQuery& BaseJob::query() const
 {
     return d->requestQuery;
 }
@@ -111,7 +112,7 @@ void BaseJob::setRequestQuery(const QUrlQuery& query)
     d->requestQuery = query;
 }
 
-const BaseJob::Data&BaseJob::requestData() const
+const BaseJob::Data& BaseJob::requestData() const
 {
     return d->requestData;
 }
diff --git a/jobs/basejob.h b/jobs/basejob.h
index ed35e3e8..460af0fc 100644
--- a/jobs/basejob.h
+++ b/jobs/basejob.h
@@ -100,7 +100,7 @@ namespace QMatrixClient
             {
                 public:
                     Status(StatusCode c) : code(c) { }
-                    Status(int c, QString m) : code(c), message(m) { }
+                    Status(int c, QString m) : code(c), message(std::move(m)) { }
 
                     bool good() const { return code < ErrorLevel; }
 
@@ -111,10 +111,10 @@ namespace QMatrixClient
             using duration_t = int; // milliseconds
 
         public:
-            BaseJob(ConnectionData* connection, HttpVerb verb, QString name,
-                    QString endpoint, Query query = Query(), Data data = Data(),
+            BaseJob(const ConnectionData* connection, HttpVerb verb,
+                    const QString& name, const QString& endpoint,
+                    const Query& query = {}, const Data& data = {},
                     bool needsToken = true);
-            virtual ~BaseJob();
 
             Status status() const;
             int error() const;
@@ -198,7 +198,7 @@ namespace QMatrixClient
             void failure(BaseJob*);
 
         protected:
-            ConnectionData* connection() const;
+            const ConnectionData* connection() const;
 
             const QUrlQuery& query() const;
             void setRequestQuery(const QUrlQuery& query);
@@ -241,6 +241,8 @@ namespace QMatrixClient
             void setStatus(Status s);
             void setStatus(int code, QString message);
 
+            // Job objects should only be deleted via QObject::deleteLater
+            virtual ~BaseJob();
         protected slots:
             void timeout();
             void sslErrors(const QList<QSslError>& errors);
diff --git a/jobs/checkauthmethods.cpp b/jobs/checkauthmethods.cpp
index 5c6a95d2..95b9a8f2 100644
--- a/jobs/checkauthmethods.cpp
+++ b/jobs/checkauthmethods.cpp
@@ -18,25 +18,18 @@
 
 #include "checkauthmethods.h"
 
-#include <QtNetwork/QNetworkAccessManager>
-#include <QtNetwork/QNetworkReply>
 #include <QtCore/QJsonDocument>
 #include <QtCore/QJsonObject>
-#include <QtCore/QJsonParseError>
-
-#include "../connectiondata.h"
 
 using namespace QMatrixClient;
 
 class CheckAuthMethods::Private
 {
     public:
-        Private() {}
-        
         QString session;
 };
 
-CheckAuthMethods::CheckAuthMethods(ConnectionData* connection)
+CheckAuthMethods::CheckAuthMethods(const ConnectionData* connection)
     : BaseJob(connection, HttpVerb::Get, "CheckAuthMethods",
               "_matrix/client/r0/login", Query(), Data(), false)
     , d(new Private)
diff --git a/jobs/checkauthmethods.h b/jobs/checkauthmethods.h
index f6eb978a..7d7dc40f 100644
--- a/jobs/checkauthmethods.h
+++ b/jobs/checkauthmethods.h
@@ -22,12 +22,10 @@
 
 namespace QMatrixClient
 {
-    class ConnectionData;
-
     class CheckAuthMethods : public BaseJob
     {
         public:
-            CheckAuthMethods(ConnectionData* connection);
+            CheckAuthMethods(const ConnectionData* connection);
             virtual ~CheckAuthMethods();
 
             QString session();
diff --git a/jobs/joinroomjob.cpp b/jobs/joinroomjob.cpp
index dce1f54e..6278c18b 100644
--- a/jobs/joinroomjob.cpp
+++ b/jobs/joinroomjob.cpp
@@ -19,10 +19,6 @@
 #include "joinroomjob.h"
 #include "util.h"
 
-#include <QtNetwork/QNetworkReply>
-
-#include "../connectiondata.h"
-
 using namespace QMatrixClient;
 
 class JoinRoomJob::Private
@@ -31,7 +27,7 @@ class JoinRoomJob::Private
         QString roomId;
 };
 
-JoinRoomJob::JoinRoomJob(ConnectionData* data, QString roomAlias)
+JoinRoomJob::JoinRoomJob(const ConnectionData* data, const QString& roomAlias)
     : BaseJob(data, HttpVerb::Post, "JoinRoomJob",
               QString("_matrix/client/r0/join/%1").arg(roomAlias))
     , d(new Private)
diff --git a/jobs/joinroomjob.h b/jobs/joinroomjob.h
index a69843ed..7cf90fd5 100644
--- a/jobs/joinroomjob.h
+++ b/jobs/joinroomjob.h
@@ -22,21 +22,19 @@
 
 namespace QMatrixClient
 {
-    class ConnectionData;
-
     class JoinRoomJob: public BaseJob
     {
         public:
-            JoinRoomJob(ConnectionData* data, QString roomAlias);
+            JoinRoomJob(const ConnectionData* data, const QString& roomAlias);
             virtual ~JoinRoomJob();
 
             QString roomId();
 
-            protected:
-                Status parseJson(const QJsonDocument& data) override;
+        protected:
+            Status parseJson(const QJsonDocument& data) override;
 
-            private:
-                class Private;
-                Private* d;
+        private:
+            class Private;
+            Private* d;
     };
-}
+}  // namespace QMatrixClient
diff --git a/jobs/leaveroomjob.cpp b/jobs/leaveroomjob.cpp
index 22a5d34b..5557b8e4 100644
--- a/jobs/leaveroomjob.cpp
+++ b/jobs/leaveroomjob.cpp
@@ -22,10 +22,7 @@
 
 using namespace QMatrixClient;
 
-LeaveRoomJob::LeaveRoomJob(ConnectionData* data, Room* room)
+LeaveRoomJob::LeaveRoomJob(const ConnectionData* data, Room* room)
     : BaseJob(data, HttpVerb::Post, "LeaveRoomJob",
               QString("_matrix/client/r0/rooms/%1/leave").arg(room->id()))
 { }
-
-LeaveRoomJob::~LeaveRoomJob()
-{ }
diff --git a/jobs/leaveroomjob.h b/jobs/leaveroomjob.h
index 4a62810f..03851f4c 100644
--- a/jobs/leaveroomjob.h
+++ b/jobs/leaveroomjob.h
@@ -22,13 +22,11 @@
 
 namespace QMatrixClient
 {
-    class ConnectionData;
     class Room;
 
     class LeaveRoomJob: public BaseJob
     {
         public:
-            LeaveRoomJob(ConnectionData* data, Room* room);
-            virtual ~LeaveRoomJob();
+            LeaveRoomJob(const ConnectionData* data, Room* room);
     };
 }
diff --git a/jobs/logoutjob.cpp b/jobs/logoutjob.cpp
index 9b9cacb6..84e88760 100644
--- a/jobs/logoutjob.cpp
+++ b/jobs/logoutjob.cpp
@@ -20,11 +20,7 @@
 
 using namespace QMatrixClient;
 
-LogoutJob::LogoutJob(ConnectionData* connection)
+LogoutJob::LogoutJob(const ConnectionData* connection)
     : BaseJob(connection, HttpVerb::Post, "LogoutJob", "/_matrix/client/r0/logout")
 {
 }
-
-LogoutJob::~LogoutJob()
-{
-}
diff --git a/jobs/logoutjob.h b/jobs/logoutjob.h
index 7d70a74c..780719e4 100644
--- a/jobs/logoutjob.h
+++ b/jobs/logoutjob.h
@@ -25,7 +25,6 @@ namespace QMatrixClient
     class LogoutJob: public BaseJob
     {
         public:
-            LogoutJob(ConnectionData* connection);
-            virtual ~LogoutJob();
+            explicit LogoutJob(const ConnectionData* connection);
     };
 }
diff --git a/jobs/mediathumbnailjob.cpp b/jobs/mediathumbnailjob.cpp
index cfde902a..9bb731b9 100644
--- a/jobs/mediathumbnailjob.cpp
+++ b/jobs/mediathumbnailjob.cpp
@@ -29,7 +29,7 @@ class MediaThumbnailJob::Private
         QPixmap thumbnail;
 };
 
-MediaThumbnailJob::MediaThumbnailJob(ConnectionData* data, QUrl url, QSize requestedSize,
+MediaThumbnailJob::MediaThumbnailJob(const ConnectionData* data, QUrl url, QSize requestedSize,
                                      ThumbnailType thumbnailType)
     : BaseJob(data, HttpVerb::Get, "MediaThumbnailJob",
               QString("/_matrix/media/v1/thumbnail/%1%2").arg(url.host(), url.path()),
diff --git a/jobs/mediathumbnailjob.h b/jobs/mediathumbnailjob.h
index cf1e9afb..307d0a99 100644
--- a/jobs/mediathumbnailjob.h
+++ b/jobs/mediathumbnailjob.h
@@ -29,7 +29,7 @@ namespace QMatrixClient
     class MediaThumbnailJob: public BaseJob
     {
         public:
-            MediaThumbnailJob(ConnectionData* data, QUrl url, QSize requestedSize,
+            MediaThumbnailJob(const ConnectionData* data, QUrl url, QSize requestedSize,
                               ThumbnailType thumbnailType=ThumbnailType::Scale);
             virtual ~MediaThumbnailJob();
 
diff --git a/jobs/passwordlogin.cpp b/jobs/passwordlogin.cpp
index 2f34e86a..081e19bc 100644
--- a/jobs/passwordlogin.cpp
+++ b/jobs/passwordlogin.cpp
@@ -18,10 +18,6 @@
 
 #include "passwordlogin.h"
 
-#include <QtNetwork/QNetworkReply>
-
-#include "../connectiondata.h"
-
 using namespace QMatrixClient;
 
 class PasswordLogin::Private
@@ -32,7 +28,7 @@ class PasswordLogin::Private
         QString returned_token;
 };
 
-PasswordLogin::PasswordLogin(ConnectionData* connection, QString user, QString password)
+PasswordLogin::PasswordLogin(const ConnectionData* connection, QString user, QString password)
     : BaseJob(connection, HttpVerb::Post, "PasswordLogin"
             , "_matrix/client/r0/login"
             , Query()
diff --git a/jobs/passwordlogin.h b/jobs/passwordlogin.h
index c1291389..713a1821 100644
--- a/jobs/passwordlogin.h
+++ b/jobs/passwordlogin.h
@@ -22,12 +22,11 @@
 
 namespace QMatrixClient
 {
-    class ConnectionData;
-
     class PasswordLogin : public BaseJob
     {
         public:
-            PasswordLogin(ConnectionData* connection, QString user, QString password);
+            PasswordLogin(const ConnectionData* connection,
+                          QString user, QString password);
             virtual ~PasswordLogin();
 
             QString token();
diff --git a/jobs/postmessagejob.cpp b/jobs/postmessagejob.cpp
index 9a102325..df30614c 100644
--- a/jobs/postmessagejob.cpp
+++ b/jobs/postmessagejob.cpp
@@ -17,33 +17,29 @@
  */
 
 #include "postmessagejob.h"
-#include "../connectiondata.h"
 #include "util.h"
 
-#include <QtNetwork/QNetworkReply>
-
 using namespace QMatrixClient;
 
 class PostMessageJob::Private
 {
     public:
-        Private() {}
-
         QString eventId; // unused yet
 };
 
-PostMessageJob::PostMessageJob(ConnectionData* connection, const QString& roomId,
-                               const QString& type, const QString& plainText)
+PostMessageJob::PostMessageJob(const ConnectionData* connection,
+                               const QString& roomId, const QString& type,
+                               const QString& plainText)
     : BaseJob(connection, HttpVerb::Post, "PostMessageJob",
-              QString("_matrix/client/r0/rooms/%1/send/m.room.message").arg(roomId),
+              QStringLiteral("_matrix/client/r0/rooms/%1/send/m.room.message").arg(roomId),
               Query(),
               Data({ { "msgtype", type }, { "body", plainText } }) )
     , d(new Private)
 { }
 
-PostMessageJob::PostMessageJob(ConnectionData* connection, const QString& roomId,
-                               const QString& type, const QString& plainText,
-                               const QString& richText)
+PostMessageJob::PostMessageJob(const ConnectionData* connection,
+                               const QString& roomId, const QString& type,
+                               const QString& plainText, const QString& richText)
     : BaseJob(connection, HttpVerb::Post, "PostMessageJob",
               QStringLiteral("_matrix/client/r0/rooms/%1/send/m.room.message").arg(roomId),
               Query(),
diff --git a/jobs/postmessagejob.h b/jobs/postmessagejob.h
index 14de52f0..f4ae809b 100644
--- a/jobs/postmessagejob.h
+++ b/jobs/postmessagejob.h
@@ -26,10 +26,10 @@ namespace QMatrixClient
     {
         public:
             /** Constructs a plain text message job */
-            PostMessageJob(ConnectionData* connection, const QString& roomId,
+            PostMessageJob(const ConnectionData* connection, const QString& roomId,
                            const QString& type, const QString& plainText);
             /** Constructs a rich text message job */
-            PostMessageJob(ConnectionData* connection, const QString& roomId,
+            PostMessageJob(const ConnectionData* connection, const QString& roomId,
                            const QString& type, const QString& plainText,
                            const QString& richText);
             virtual ~PostMessageJob();
@@ -43,4 +43,4 @@ namespace QMatrixClient
             class Private;
             Private* d;
     };
-}
+}  // namespace QMatrixClient
diff --git a/jobs/postreceiptjob.cpp b/jobs/postreceiptjob.cpp
index ee750dbf..00926de6 100644
--- a/jobs/postreceiptjob.cpp
+++ b/jobs/postreceiptjob.cpp
@@ -17,17 +17,11 @@
  */
 
 #include "postreceiptjob.h"
-#include "../room.h"
-#include "../connectiondata.h"
-
-#include <QtNetwork/QNetworkReply>
 
 using namespace QMatrixClient;
 
-PostReceiptJob::PostReceiptJob(ConnectionData* connection, QString roomId, QString eventId)
+PostReceiptJob::PostReceiptJob(const ConnectionData* connection,
+                               const QString& roomId, const QString& eventId)
     : BaseJob(connection, HttpVerb::Post, "PostReceiptJob",
               QString("/_matrix/client/r0/rooms/%1/receipt/m.read/%2").arg(roomId, eventId))
 { }
-
-PostReceiptJob::~PostReceiptJob()
-{ }
diff --git a/jobs/postreceiptjob.h b/jobs/postreceiptjob.h
index c0002dc0..1c84f411 100644
--- a/jobs/postreceiptjob.h
+++ b/jobs/postreceiptjob.h
@@ -25,7 +25,7 @@ namespace QMatrixClient
     class PostReceiptJob: public BaseJob
     {
         public:
-            PostReceiptJob(ConnectionData* connection, QString roomId, QString eventId);
-            virtual ~PostReceiptJob();
+            PostReceiptJob(const ConnectionData* connection, const QString& roomId,
+                           const QString& eventId);
     };
 }
diff --git a/jobs/roommessagesjob.cpp b/jobs/roommessagesjob.cpp
index 5779d695..a48403c8 100644
--- a/jobs/roommessagesjob.cpp
+++ b/jobs/roommessagesjob.cpp
@@ -26,14 +26,12 @@ using namespace QMatrixClient;
 class RoomMessagesJob::Private
 {
     public:
-        Private() {}
-
         Owning<Events> events;
         QString end;
 };
 
-RoomMessagesJob::RoomMessagesJob(ConnectionData* data, QString roomId,
-                                 QString from, int limit, FetchDirection dir)
+RoomMessagesJob::RoomMessagesJob(const ConnectionData* data, const QString& roomId,
+                                 const QString& from, int limit, FetchDirection dir)
     : BaseJob(data, HttpVerb::Get, "RoomMessagesJob",
               QString("/_matrix/client/r0/rooms/%1/messages").arg(roomId),
               Query(
diff --git a/jobs/roommessagesjob.h b/jobs/roommessagesjob.h
index af7d65df..2d15d9d4 100644
--- a/jobs/roommessagesjob.h
+++ b/jobs/roommessagesjob.h
@@ -29,8 +29,8 @@ namespace QMatrixClient
     class RoomMessagesJob: public BaseJob
     {
         public:
-            RoomMessagesJob(ConnectionData* data, QString roomId,
-                            QString from, int limit = 10,
+            RoomMessagesJob(const ConnectionData* data, const QString& roomId,
+                            const QString& from, int limit = 10,
                             FetchDirection dir = FetchDirection::Backward);
             virtual ~RoomMessagesJob();
 
@@ -44,4 +44,4 @@ namespace QMatrixClient
             class Private;
             Private* d;
     };
-}
+}  // namespace QMatrixClient
diff --git a/jobs/syncjob.cpp b/jobs/syncjob.cpp
index 8db1e2ca..17c637b0 100644
--- a/jobs/syncjob.cpp
+++ b/jobs/syncjob.cpp
@@ -32,8 +32,8 @@ class SyncJob::Private
 
 static size_t jobId = 0;
 
-SyncJob::SyncJob(ConnectionData* connection,
-                 QString since, QString filter, int timeout, QString presence)
+SyncJob::SyncJob(const ConnectionData* connection, const QString& since,
+                 const QString& filter, int timeout, const QString& presence)
     : BaseJob(connection, HttpVerb::Get, QString("SyncJob-%1").arg(++jobId),
               "_matrix/client/r0/sync")
     , d(new Private)
@@ -84,11 +84,10 @@ BaseJob::Status SyncJob::parseJson(const QJsonDocument& data)
     for (auto roomState: roomStates)
     {
         const QJsonObject rs = rooms.value(roomState.jsonKey).toObject();
-        d->roomData.reserve(rs.size());
+        // We have a Qt container on the right and an STL one on the left
+        d->roomData.reserve(static_cast<size_t>(rs.size()));
         for( auto rkey: rs.keys() )
-        {
-            d->roomData.push_back({rkey, roomState.enumVal, rs[rkey].toObject()});
-        }
+            d->roomData.emplace_back(rkey, roomState.enumVal, rs[rkey].toObject());
     }
 
     return Success;
@@ -99,7 +98,8 @@ void SyncRoomData::EventList::fromJson(const QJsonObject& roomContents)
     assign(eventsFromJson(roomContents[jsonKey].toObject()["events"].toArray()));
 }
 
-SyncRoomData::SyncRoomData(QString roomId_, JoinState joinState_, const QJsonObject& room_)
+SyncRoomData::SyncRoomData(const QString& roomId_, JoinState joinState_,
+                           const QJsonObject& room_)
     : roomId(roomId_)
     , joinState(joinState_)
     , state("state")
diff --git a/jobs/syncjob.h b/jobs/syncjob.h
index b41c09d4..21d3cfca 100644
--- a/jobs/syncjob.h
+++ b/jobs/syncjob.h
@@ -34,7 +34,7 @@ namespace QMatrixClient
             private:
                 QString jsonKey;
             public:
-                explicit EventList(QString k) : jsonKey(k) { }
+                explicit EventList(QString k) : jsonKey(std::move(k)) { }
                 void fromJson(const QJsonObject& roomContents);
         };
 
@@ -51,11 +51,10 @@ namespace QMatrixClient
         int highlightCount;
         int notificationCount;
 
-        SyncRoomData(QString roomId_ = QString(),
-                     JoinState joinState_ = JoinState::Join,
-                     const QJsonObject& room_ = QJsonObject());
+        SyncRoomData(const QString& roomId, JoinState joinState_,
+                     const QJsonObject& room_);
     };
-}
+}  // namespace QMatrixClient
 Q_DECLARE_TYPEINFO(QMatrixClient::SyncRoomData, Q_MOVABLE_TYPE);
 
 namespace QMatrixClient
@@ -63,12 +62,12 @@ namespace QMatrixClient
     // QVector cannot work with non-copiable objects, std::vector can.
     using SyncData = std::vector<SyncRoomData>;
 
-    class ConnectionData;
     class SyncJob: public BaseJob
     {
         public:
-            SyncJob(ConnectionData* connection, QString since = {}, QString filter = {},
-                    int timeout = -1, QString presence = {});
+            explicit SyncJob(const ConnectionData* connection, const QString& since = {},
+                             const QString& filter = {},
+                             int timeout = -1, const QString& presence = {});
             virtual ~SyncJob();
 
             SyncData& roomData();
@@ -81,4 +80,4 @@ namespace QMatrixClient
             class Private;
             Private* d;
     };
-}
+}  // namespace QMatrixClient