{"action":"created","issue":{"url":"https://api.github.com/repos/mixxxdj/mixxx/issues/1161","repository_url":"https://api.github.com/repos/mixxxdj/mixxx","labels_url":"https://api.github.com/repos/mixxxdj/mixxx/issues/1161/labels{/name}","comments_url":"https://api.github.com/repos/mixxxdj/mixxx/issues/1161/comments","events_url":"https://api.github.com/repos/mixxxdj/mixxx/issues/1161/events","html_url":"https://github.com/mixxxdj/mixxx/pull/1161","id":203872814,"number":1161,"title":"parented_weak_ptr","user":{"login":"timrae","id":2818274,"avatar_url":"https://avatars.githubusercontent.com/u/2818274?v=3","gravatar_id":"","url":"https://api.github.com/users/timrae","html_url":"https://github.com/timrae","followers_url":"https://api.github.com/users/timrae/followers","following_url":"https://api.github.com/users/timrae/following{/other_user}","gists_url":"https://api.github.com/users/timrae/gists{/gist_id}","starred_url":"https://api.github.com/users/timrae/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/timrae/subscriptions","organizations_url":"https://api.github.com/users/timrae/orgs","repos_url":"https://api.github.com/users/timrae/repos","events_url":"https://api.github.com/users/timrae/events{/privacy}","received_events_url":"https://api.github.com/users/timrae/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":19,"created_at":"2017-01-29T14:24:16Z","updated_at":"2017-01-31T00:07:24Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/mixxxdj/mixxx/pulls/1161","html_url":"https://github.com/mixxxdj/mixxx/pull/1161","diff_url":"https://github.com/mixxxdj/mixxx/pull/1161.diff","patch_url":"https://github.com/mixxxdj/mixxx/pull/1161.patch"},"body":"Following on from the discussion in #1142, having the copy constructor available in the `parented_ptr` class gives an incorrect feeling that it could be used in the same way as a `shared_ptr`, which would lead to dangling pointers if the pointer can outlive its parent.\r\n\r\nTo address this problem we introduce a second class `parented_weak_ptr` that wraps the raw pointer in a QPointer. Since @rryan pointed out that QPointer has some overhead, we keep the old `parented_ptr` which doesn't use QPointer, but change it so that it can only be used with move semantics, making it safer than the version in #1142 in that the compiler will detect some (but not all) possible abuses.\r\n\r\nBasically the intention with this class is that the \"owner\" (of the `parented_ptr`) and the \"parent\" are the same object. In such cases the overhead of `QPointer` is unwarranted:\r\n\r\n```c++\r\nclass ParentClass {\r\n  private:\r\n    parented_ptr<T> m_pChild = std::make_parented<T>(this);\r\n};\r\n```\r\n\r\nThe changes in this PR are working well in practice, as you can see in c9eaaf47d142d90cb538baedb27750e6eb31d71e.\r\n\r\nNote that I [initially](https://gist.github.com/timrae/f6cca4af6a9ad8d9d45e76b0e23ab909) tried to make a version of `parented_ptr` that inherits from `unique_ptr` to minimize duplicated code, but I couldn't get that to work, as it was complaining about the deleted copy constructor when initializing `m_pLibraryControl` in c9eaaf47d142d90cb538baedb27750e6eb31d71e. If someone can figure out the trick how to get that to work, I think it would be worth changing to such an implementation."},"comment":{"url":"https://api.github.com/repos/mixxxdj/mixxx/issues/comments/276232937","html_url":"https://github.com/mixxxdj/mixxx/pull/1161#issuecomment-276232937","issue_url":"https://api.github.com/repos/mixxxdj/mixxx/issues/1161","id":276232937,"user":{"login":"timrae","id":2818274,"avatar_url":"https://avatars.githubusercontent.com/u/2818274?v=3","gravatar_id":"","url":"https://api.github.com/users/timrae","html_url":"https://github.com/timrae","followers_url":"https://api.github.com/users/timrae/followers","following_url":"https://api.github.com/users/timrae/following{/other_user}","gists_url":"https://api.github.com/users/timrae/gists{/gist_id}","starred_url":"https://api.github.com/users/timrae/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/timrae/subscriptions","organizations_url":"https://api.github.com/users/timrae/orgs","repos_url":"https://api.github.com/users/timrae/repos","events_url":"https://api.github.com/users/timrae/events{/privacy}","received_events_url":"https://api.github.com/users/timrae/received_events","type":"User","site_admin":false},"created_at":"2017-01-31T00:07:24Z","updated_at":"2017-01-31T00:07:24Z","body":"> We can consider to use always parented_weak_ptr after 2.1 is released. But I am not feeling well to introduce a useless heap allocation and reference counting to clean it up, just because of an self made programming pattern.\r\n\r\n> A parented_weak_ptr alone gives us that.\r\n\r\nYou seem to be contradicting yourself in the same post... I agree with the first point, which makes the second point redundant. I think we agree that `parented_weak_ptr` is good, but not enough. So now we need to make the new unsafe/raw pointer as safe and useful as we can, and we do this by introducing clear and easily enforceable rules on its usage. Preferably the rules should be enforced by the compiler when this is possible.\r\n\r\nThe rule you previously suggested was:\r\n\r\n* Only use `parented_raw_ptr` when the parent outlives the pointer.\r\n\r\nWhile this is correct and sufficient to prevent dangling pointers, it's not easily enforceable. You may have to hunt through the code to track the lifetime of the pointer and the parent, and then come to a conclusion about whether or not it's safe. If you allow the pointer to be copyable then you drastically increase this work load, drastically increase the chance of making a mistake, and drastically increase the chance that the code will be broken later by someone else who didn't understand it as well as you.\r\n\r\n*If your use case is sufficiently complex that you need to copy the pointer, then you should be using `parented_weak_ptr`*"}}
{"actor":{"display_login":"timrae"}}