{"action":"created","comment":{"url":"https://api.github.com/repos/tux3/qTox/pulls/comments/68405180","id":68405180,"diff_hunk":"@@ -492,7 +492,7 @@ void Settings::savePersonal(Profile* profile)\n     savePersonal(profile->getName(), profile->getPassword());\n }\n \n-void Settings::savePersonal(QString profileName, QString password)\n+void Settings::savePersonal(QString profileName, const QString &password)","path":"src/persistence/settings.cpp","position":5,"original_position":5,"commit_id":"e852ed92f6a83d30d824cf3b35a8d3f68537fe9c","original_commit_id":"e852ed92f6a83d30d824cf3b35a8d3f68537fe9c","user":{"login":"PafXUS","id":19802901,"avatar_url":"https://avatars.githubusercontent.com/u/19802901?v=3","gravatar_id":"","url":"https://api.github.com/users/PafXUS","html_url":"https://github.com/PafXUS","followers_url":"https://api.github.com/users/PafXUS/followers","following_url":"https://api.github.com/users/PafXUS/following{/other_user}","gists_url":"https://api.github.com/users/PafXUS/gists{/gist_id}","starred_url":"https://api.github.com/users/PafXUS/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/PafXUS/subscriptions","organizations_url":"https://api.github.com/users/PafXUS/orgs","repos_url":"https://api.github.com/users/PafXUS/repos","events_url":"https://api.github.com/users/PafXUS/events{/privacy}","received_events_url":"https://api.github.com/users/PafXUS/received_events","type":"User","site_admin":false},"body":"@tux3 I agree that we should be very careful. But, i don't agree that const QString & will introduce use-after-free problem. Why so? Because - all \"in\" arguments by default are copy-constructed on caller side and their lifetime is connected to event not to original object from call. You can find in [Qt docs](https://doc.qt.io/qt-5.7/qmetaobject.html#invokeMethod): \r\n\r\n> With asynchronous method invocations, the **parameters** must be of types that are known to Qt's meta-object system, because **Qt needs to copy the arguments to store them** in an event behind the scenes. If you try to use a queued connection and get the error message \r\n\r\nAnd there were at least 2 separate discussions about this behavior [about signal-slots in general](http://www.embeddeduse.com/2013/06/29/copied-or-not-copied-arguments-signals-slots/) and about [invokeMethod ](http://www.qtcentre.org/threads/46934-Using-QMetaObject-invokeMethod-on-a-SLOT-that-has-a-QString-Reference-argument?s=680ce9ced4a4b0ef87d82615926d0071) \r\n So in short: no, there is no any harm by doing const QString & instead of  QString in that case. Or can you please bring some small illustration in case of some misunderstanding on my part, of what exactly will happen. ","created_at":"2016-06-24T14:22:59Z","updated_at":"2016-06-24T14:22:59Z","html_url":"https://github.com/tux3/qTox/pull/3411#discussion_r68405180","pull_request_url":"https://api.github.com/repos/tux3/qTox/pulls/3411","_links":{"self":{"href":"https://api.github.com/repos/tux3/qTox/pulls/comments/68405180"},"html":{"href":"https://github.com/tux3/qTox/pull/3411#discussion_r68405180"},"pull_request":{"href":"https://api.github.com/repos/tux3/qTox/pulls/3411"}}},"pull_request":{"url":"https://api.github.com/repos/tux3/qTox/pulls/3411","id":74328447,"html_url":"https://github.com/tux3/qTox/pull/3411","diff_url":"https://github.com/tux3/qTox/pull/3411.diff","patch_url":"https://github.com/tux3/qTox/pull/3411.patch","issue_url":"https://api.github.com/repos/tux3/qTox/issues/3411","number":3411,"state":"open","locked":false,"title":"fix(persistence): pass by reference to const","user":{"login":"PafXUS","id":19802901,"avatar_url":"https://avatars.githubusercontent.com/u/19802901?v=3","gravatar_id":"","url":"https://api.github.com/users/PafXUS","html_url":"https://github.com/PafXUS","followers_url":"https://api.github.com/users/PafXUS/followers","following_url":"https://api.github.com/users/PafXUS/following{/other_user}","gists_url":"https://api.github.com/users/PafXUS/gists{/gist_id}","starred_url":"https://api.github.com/users/PafXUS/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/PafXUS/subscriptions","organizations_url":"https://api.github.com/users/PafXUS/orgs","repos_url":"https://api.github.com/users/PafXUS/repos","events_url":"https://api.github.com/users/PafXUS/events{/privacy}","received_events_url":"https://api.github.com/users/PafXUS/received_events","type":"User","site_admin":false},"body":"This commit changes passing by value behavior for password occurrences to pass by reference to const.\r\nI've noticed great inconsistency in codebase about how \"in\" parameters of large (with sizeof(T) > 8 bytes) non-trivial types are passed around. For example QString, QByteArray, QList, std::function, IPCEventHandler, ToxFile sometimes are passed around by value, sometimes, by reference to const. I think all of them should be passed by reference to const by default. This advice can be found in [CoreGuidelines](https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#f16-for-in-parameters-pass-cheaply-copied-types-by-value-and-others-by-reference-to-const). This must be applied to Qt types with implicit sharing as well (you can see how Qt library itself pass it's types). Exceptions to this can be intentional moving around types, [copy-and-swap idiom](https://en.wikibooks.org/wiki/More_C%2B%2B_Idioms/Copy-and-swap), or copy for modification.\r\n   If everybody agree on this, i can help with fixing ~400 occurrences of this issue in codebase. Please discuss.","created_at":"2016-06-18T05:33:24Z","updated_at":"2016-06-24T14:22:59Z","closed_at":null,"merged_at":null,"merge_commit_sha":"6dbec2f70598bf146bd05ed14524ebbb46614ab3","assignee":null,"assignees":[],"milestone":null,"commits_url":"https://api.github.com/repos/tux3/qTox/pulls/3411/commits","review_comments_url":"https://api.github.com/repos/tux3/qTox/pulls/3411/comments","review_comment_url":"https://api.github.com/repos/tux3/qTox/pulls/comments{/number}","comments_url":"https://api.github.com/repos/tux3/qTox/issues/3411/comments","statuses_url":"https://api.github.com/repos/tux3/qTox/statuses/e852ed92f6a83d30d824cf3b35a8d3f68537fe9c","head":{"label":"PafXUS:performance_improvements","ref":"performance_improvements","sha":"e852ed92f6a83d30d824cf3b35a8d3f68537fe9c","user":{"login":"PafXUS","id":19802901,"avatar_url":"https://avatars.githubusercontent.com/u/19802901?v=3","gravatar_id":"","url":"https://api.github.com/users/PafXUS","html_url":"https://github.com/PafXUS","followers_url":"https://api.github.com/users/PafXUS/followers","following_url":"https://api.github.com/users/PafXUS/following{/other_user}","gists_url":"https://api.github.com/users/PafXUS/gists{/gist_id}","starred_url":"https://api.github.com/users/PafXUS/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/PafXUS/subscriptions","organizations_url":"https://api.github.com/users/PafXUS/orgs","repos_url":"https://api.github.com/users/PafXUS/repos","events_url":"https://api.github.com/users/PafXUS/events{/privacy}","received_events_url":"https://api.github.com/users/PafXUS/received_events","type":"User","site_admin":false},"repo":{"id":60630311,"name":"qTox","full_name":"PafXUS/qTox","owner":{"login":"PafXUS","id":19802901,"avatar_url":"https://avatars.githubusercontent.com/u/19802901?v=3","gravatar_id":"","url":"https://api.github.com/users/PafXUS","html_url":"https://github.com/PafXUS","followers_url":"https://api.github.com/users/PafXUS/followers","following_url":"https://api.github.com/users/PafXUS/following{/other_user}","gists_url":"https://api.github.com/users/PafXUS/gists{/gist_id}","starred_url":"https://api.github.com/users/PafXUS/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/PafXUS/subscriptions","organizations_url":"https://api.github.com/users/PafXUS/orgs","repos_url":"https://api.github.com/users/PafXUS/repos","events_url":"https://api.github.com/users/PafXUS/events{/privacy}","received_events_url":"https://api.github.com/users/PafXUS/received_events","type":"User","site_admin":false},"private":false,"html_url":"https://github.com/PafXUS/qTox","description":"Powerful Tox chat client that follows the Tox design guidelines.","fork":true,"url":"https://api.github.com/repos/PafXUS/qTox","forks_url":"https://api.github.com/repos/PafXUS/qTox/forks","keys_url":"https://api.github.com/repos/PafXUS/qTox/keys{/key_id}","collaborators_url":"https://api.github.com/repos/PafXUS/qTox/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/PafXUS/qTox/teams","hooks_url":"https://api.github.com/repos/PafXUS/qTox/hooks","issue_events_url":"https://api.github.com/repos/PafXUS/qTox/issues/events{/number}","events_url":"https://api.github.com/repos/PafXUS/qTox/events","assignees_url":"https://api.github.com/repos/PafXUS/qTox/assignees{/user}","branches_url":"https://api.github.com/repos/PafXUS/qTox/branches{/branch}","tags_url":"https://api.github.com/repos/PafXUS/qTox/tags","blobs_url":"https://api.github.com/repos/PafXUS/qTox/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/PafXUS/qTox/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/PafXUS/qTox/git/refs{/sha}","trees_url":"https://api.github.com/repos/PafXUS/qTox/git/trees{/sha}","statuses_url":"https://api.github.com/repos/PafXUS/qTox/statuses/{sha}","languages_url":"https://api.github.com/repos/PafXUS/qTox/languages","stargazers_url":"https://api.github.com/repos/PafXUS/qTox/stargazers","contributors_url":"https://api.github.com/repos/PafXUS/qTox/contributors","subscribers_url":"https://api.github.com/repos/PafXUS/qTox/subscribers","subscription_url":"https://api.github.com/repos/PafXUS/qTox/subscription","commits_url":"https://api.github.com/repos/PafXUS/qTox/commits{/sha}","git_commits_url":"https://api.github.com/repos/PafXUS/qTox/git/commits{/sha}","comments_url":"https://api.github.com/repos/PafXUS/qTox/comments{/number}","issue_comment_url":"https://api.github.com/repos/PafXUS/qTox/issues/comments{/number}","contents_url":"https://api.github.com/repos/PafXUS/qTox/contents/{+path}","compare_url":"https://api.github.com/repos/PafXUS/qTox/compare/{base}...{head}","merges_url":"https://api.github.com/repos/PafXUS/qTox/merges","archive_url":"https://api.github.com/repos/PafXUS/qTox/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/PafXUS/qTox/downloads","issues_url":"https://api.github.com/repos/PafXUS/qTox/issues{/number}","pulls_url":"https://api.github.com/repos/PafXUS/qTox/pulls{/number}","milestones_url":"https://api.github.com/repos/PafXUS/qTox/milestones{/number}","notifications_url":"https://api.github.com/repos/PafXUS/qTox/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/PafXUS/qTox/labels{/name}","releases_url":"https://api.github.com/repos/PafXUS/qTox/releases{/id}","deployments_url":"https://api.github.com/repos/PafXUS/qTox/deployments","created_at":"2016-06-07T16:49:03Z","updated_at":"2016-06-07T16:49:06Z","pushed_at":"2016-06-18T05:05:02Z","git_url":"git://github.com/PafXUS/qTox.git","ssh_url":"git@github.com:PafXUS/qTox.git","clone_url":"https://github.com/PafXUS/qTox.git","svn_url":"https://github.com/PafXUS/qTox","homepage":"https://qtox.github.io/","size":19759,"stargazers_count":0,"watchers_count":0,"language":"C++","has_issues":false,"has_downloads":true,"has_wiki":true,"has_pages":false,"forks_count":0,"mirror_url":null,"open_issues_count":0,"forks":0,"open_issues":0,"watchers":0,"default_branch":"master"}},"base":{"label":"tux3:master","ref":"master","sha":"e3278853d50a6df063fa3f3f451037bdb9f0c7c0","user":{"login":"tux3","id":5155966,"avatar_url":"https://avatars.githubusercontent.com/u/5155966?v=3","gravatar_id":"","url":"https://api.github.com/users/tux3","html_url":"https://github.com/tux3","followers_url":"https://api.github.com/users/tux3/followers","following_url":"https://api.github.com/users/tux3/following{/other_user}","gists_url":"https://api.github.com/users/tux3/gists{/gist_id}","starred_url":"https://api.github.com/users/tux3/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/tux3/subscriptions","organizations_url":"https://api.github.com/users/tux3/orgs","repos_url":"https://api.github.com/users/tux3/repos","events_url":"https://api.github.com/users/tux3/events{/privacy}","received_events_url":"https://api.github.com/users/tux3/received_events","type":"User","site_admin":false},"repo":{"id":21179164,"name":"qTox","full_name":"tux3/qTox","owner":{"login":"tux3","id":5155966,"avatar_url":"https://avatars.githubusercontent.com/u/5155966?v=3","gravatar_id":"","url":"https://api.github.com/users/tux3","html_url":"https://github.com/tux3","followers_url":"https://api.github.com/users/tux3/followers","following_url":"https://api.github.com/users/tux3/following{/other_user}","gists_url":"https://api.github.com/users/tux3/gists{/gist_id}","starred_url":"https://api.github.com/users/tux3/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/tux3/subscriptions","organizations_url":"https://api.github.com/users/tux3/orgs","repos_url":"https://api.github.com/users/tux3/repos","events_url":"https://api.github.com/users/tux3/events{/privacy}","received_events_url":"https://api.github.com/users/tux3/received_events","type":"User","site_admin":false},"private":false,"html_url":"https://github.com/tux3/qTox","description":"Powerful Tox chat client that follows the Tox design guidelines.","fork":false,"url":"https://api.github.com/repos/tux3/qTox","forks_url":"https://api.github.com/repos/tux3/qTox/forks","keys_url":"https://api.github.com/repos/tux3/qTox/keys{/key_id}","collaborators_url":"https://api.github.com/repos/tux3/qTox/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/tux3/qTox/teams","hooks_url":"https://api.github.com/repos/tux3/qTox/hooks","issue_events_url":"https://api.github.com/repos/tux3/qTox/issues/events{/number}","events_url":"https://api.github.com/repos/tux3/qTox/events","assignees_url":"https://api.github.com/repos/tux3/qTox/assignees{/user}","branches_url":"https://api.github.com/repos/tux3/qTox/branches{/branch}","tags_url":"https://api.github.com/repos/tux3/qTox/tags","blobs_url":"https://api.github.com/repos/tux3/qTox/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/tux3/qTox/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/tux3/qTox/git/refs{/sha}","trees_url":"https://api.github.com/repos/tux3/qTox/git/trees{/sha}","statuses_url":"https://api.github.com/repos/tux3/qTox/statuses/{sha}","languages_url":"https://api.github.com/repos/tux3/qTox/languages","stargazers_url":"https://api.github.com/repos/tux3/qTox/stargazers","contributors_url":"https://api.github.com/repos/tux3/qTox/contributors","subscribers_url":"https://api.github.com/repos/tux3/qTox/subscribers","subscription_url":"https://api.github.com/repos/tux3/qTox/subscription","commits_url":"https://api.github.com/repos/tux3/qTox/commits{/sha}","git_commits_url":"https://api.github.com/repos/tux3/qTox/git/commits{/sha}","comments_url":"https://api.github.com/repos/tux3/qTox/comments{/number}","issue_comment_url":"https://api.github.com/repos/tux3/qTox/issues/comments{/number}","contents_url":"https://api.github.com/repos/tux3/qTox/contents/{+path}","compare_url":"https://api.github.com/repos/tux3/qTox/compare/{base}...{head}","merges_url":"https://api.github.com/repos/tux3/qTox/merges","archive_url":"https://api.github.com/repos/tux3/qTox/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/tux3/qTox/downloads","issues_url":"https://api.github.com/repos/tux3/qTox/issues{/number}","pulls_url":"https://api.github.com/repos/tux3/qTox/pulls{/number}","milestones_url":"https://api.github.com/repos/tux3/qTox/milestones{/number}","notifications_url":"https://api.github.com/repos/tux3/qTox/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/tux3/qTox/labels{/name}","releases_url":"https://api.github.com/repos/tux3/qTox/releases{/id}","deployments_url":"https://api.github.com/repos/tux3/qTox/deployments","created_at":"2014-06-24T20:09:04Z","updated_at":"2016-06-23T20:12:37Z","pushed_at":"2016-06-24T11:07:47Z","git_url":"git://github.com/tux3/qTox.git","ssh_url":"git@github.com:tux3/qTox.git","clone_url":"https://github.com/tux3/qTox.git","svn_url":"https://github.com/tux3/qTox","homepage":"https://qtox.github.io/","size":24902,"stargazers_count":981,"watchers_count":981,"language":"C++","has_issues":true,"has_downloads":true,"has_wiki":true,"has_pages":false,"forks_count":288,"mirror_url":null,"open_issues_count":489,"forks":288,"open_issues":489,"watchers":981,"default_branch":"master"}},"_links":{"self":{"href":"https://api.github.com/repos/tux3/qTox/pulls/3411"},"html":{"href":"https://github.com/tux3/qTox/pull/3411"},"issue":{"href":"https://api.github.com/repos/tux3/qTox/issues/3411"},"comments":{"href":"https://api.github.com/repos/tux3/qTox/issues/3411/comments"},"review_comments":{"href":"https://api.github.com/repos/tux3/qTox/pulls/3411/comments"},"review_comment":{"href":"https://api.github.com/repos/tux3/qTox/pulls/comments{/number}"},"commits":{"href":"https://api.github.com/repos/tux3/qTox/pulls/3411/commits"},"statuses":{"href":"https://api.github.com/repos/tux3/qTox/statuses/e852ed92f6a83d30d824cf3b35a8d3f68537fe9c"}}}}
{"actor":{"display_login":"PafXUS"}}