{"action":"created","comment":{"url":"https://api.github.com/repos/pybind/pybind11/pulls/comments/102044635","pull_request_review_id":22788452,"id":102044635,"diff_hunk":"@@ -70,6 +70,7 @@ set(PYBIND11_HEADERS\n   include/pybind11/stl.h\n   include/pybind11/stl_bind.h\n   include/pybind11/typeid.h\n+  include/pybind11/detail/class_.h","path":"CMakeLists.txt","position":null,"original_position":4,"commit_id":"69a91ab869b54d2b150ee92d3e603f7dce4fcb20","original_commit_id":"aa5b557e566d38d3871b6c15801b530f036e8a73","user":{"login":"dean0x7d","id":4831417,"avatar_url":"https://avatars.githubusercontent.com/u/4831417?v=3","gravatar_id":"","url":"https://api.github.com/users/dean0x7d","html_url":"https://github.com/dean0x7d","followers_url":"https://api.github.com/users/dean0x7d/followers","following_url":"https://api.github.com/users/dean0x7d/following{/other_user}","gists_url":"https://api.github.com/users/dean0x7d/gists{/gist_id}","starred_url":"https://api.github.com/users/dean0x7d/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/dean0x7d/subscriptions","organizations_url":"https://api.github.com/users/dean0x7d/orgs","repos_url":"https://api.github.com/users/dean0x7d/repos","events_url":"https://api.github.com/users/dean0x7d/events{/privacy}","received_events_url":"https://api.github.com/users/dean0x7d/received_events","type":"User","site_admin":false},"body":"Yeah, this PR probably shouldn't introduce a one-off change like that. Renamed `detail/class_.h` -> `class_support.h`. I squashed back the change right away to keep the commits clean.","created_at":"2017-02-20T15:44:30Z","updated_at":"2017-02-20T15:44:30Z","html_url":"https://github.com/pybind/pybind11/pull/679#discussion_r102044635","pull_request_url":"https://api.github.com/repos/pybind/pybind11/pulls/679","_links":{"self":{"href":"https://api.github.com/repos/pybind/pybind11/pulls/comments/102044635"},"html":{"href":"https://github.com/pybind/pybind11/pull/679#discussion_r102044635"},"pull_request":{"href":"https://api.github.com/repos/pybind/pybind11/pulls/679"}}},"pull_request":{"url":"https://api.github.com/repos/pybind/pybind11/pulls/679","id":106741759,"html_url":"https://github.com/pybind/pybind11/pull/679","diff_url":"https://github.com/pybind/pybind11/pull/679.diff","patch_url":"https://github.com/pybind/pybind11/pull/679.patch","issue_url":"https://api.github.com/repos/pybind/pybind11/issues/679","number":679,"state":"open","locked":false,"title":"Fix multiple inheritance with static properties","user":{"login":"dean0x7d","id":4831417,"avatar_url":"https://avatars.githubusercontent.com/u/4831417?v=3","gravatar_id":"","url":"https://api.github.com/users/dean0x7d","html_url":"https://github.com/dean0x7d","followers_url":"https://api.github.com/users/dean0x7d/followers","following_url":"https://api.github.com/users/dean0x7d/following{/other_user}","gists_url":"https://api.github.com/users/dean0x7d/gists{/gist_id}","starred_url":"https://api.github.com/users/dean0x7d/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/dean0x7d/subscriptions","organizations_url":"https://api.github.com/users/dean0x7d/orgs","repos_url":"https://api.github.com/users/dean0x7d/repos","events_url":"https://api.github.com/users/dean0x7d/events{/privacy}","received_events_url":"https://api.github.com/users/dean0x7d/received_events","type":"User","site_admin":false},"body":"This fixes #597 and also (unintentionally) resolves #594.\r\n\r\n@pschella: This fixes the original test code that you posted in #597 and I think the fix is quite thorough. But it might be good if you could test these changes in case there are any missing corner cases.\r\n\r\n### Changes\r\n\r\nThe main issue for multiple inheritance is that Python not only requires that all base types are layout compatible, but also that they have a common metaclass. This resulted in problems when mixing classes with different `py::metaclass` and `py::dynamic_attr` specifications. This PR attempts to bring harmony to the different type hierarchies. This is a bit involved but most of the changes are organizational, not fundamental (the types are just stacked a bit differently).\r\n\r\n#### Benefits\r\n\r\n* Multiple inheritance now works with combinations of `py::metaclass`, `py::dynamic_attr` and 'vanilla' classes, independent of base list order. The new uniform layout also makes it possible to inherit from multiple C++ types in Python: defining `class PyType(CppType1, CppType2)` works just like `py::class_<T, CppType1, CppType2>`.\r\n\r\n* The new metaclass implementation is cheap (essentially free) making it possible to enable `py::metaclass` for all classes by default. `py::metaclass` is thus deprecated.\r\n\r\n* The new metaclass/static property implementation also brings a minor semantics improvement: it's now possible to access static properties via instances (`x = instance.static_prop`) where this was previously type-only (`x = Type.static_prop`).\r\n\r\n#### Cost\r\n\r\n* There's some extra global data (see `internals`) and a an extra pointer in the per-type data (`type_info`). But the per-type unique metaclass is now replaced with a single shared metaclass which reduces overall memory requirements.\r\n\r\n* Not counting the new tests, the binary size of the test module stays about the same (+140 bytes = +0.01% on clang/macOS).\r\n\r\n\r\n### Implementation\r\n\r\nI hope the following quick ASCII diagrams will help illustrate the implementation change. A general relationship between instance, type and metatype looks something like this:\r\n```\r\n               TypeBase     MetatypeBase\r\n                  |              |\r\ninstance  -->    Type   -->   Metatype\r\n                  |\r\n               Derived\r\n```\r\nHere `Type` is some user-define class and `instance = Type()`. `Metatype` is the type's type and the inheritance is shown top-down, separately for the type and the metatype.\r\n\r\nTypically, this is pretty simple in Python:\r\n```\r\n                object     \r\n                  |\r\ninstance  -->    Type  -->  type\r\n                  |\r\n               Derived\r\n```\r\nHere `object` and `type` are Python builtins and the above just illustrates the following in Python3:\r\n```python\r\nclass Type:\r\n    pass\r\n\r\nclass Derived(Type):\r\n    pass\r\n\r\ninstance = Type()\r\n```\r\n\r\nNow, the existing pybind11 implementation for classes with static properties (`py::metaclass`) looks a bit like this:\r\n```\r\n                object             type\r\n                  |                 |\r\ninstance  -->    Type   -->   UniqueMetatype\r\n                    \\                      \\\r\n                     \\-> property           \\-> property (static)\r\n```\r\nRegular properties (`instance.prop`) are stored in `Type` while static properties (`Type.prop`) are stored in a metaclass which is unique for each `Type`. This doubles the amount of data allocated per type which makes it expensive and thus guarded by `py::metaclass`.\r\n\r\nThis PR changes things around to look like this:\r\n```\r\n                object             type\r\n                  |                 |\r\ninstance  -->    Type   -->   SharedMetatype\r\n                    \\\r\n                     \\-> property\r\n                     |-> static_property\r\n```\r\nThis approach is inspired by `boost.python` but with a slimmer implementation (most notably it doesn't replicate any CPython internal structures). The basic idea is that we extend Python's builtin `property` type and adapt it for static data. The following Python code is the exact implementation, but we just create it using the C API:\r\n```python\r\nclass pybind11_static_property(property):\r\n    \"\"\"Exactly like property, but we always pass the type (`cls`)\r\n    instead of the instance (`obj`)\"\"\"\r\n\r\n    def __get__(self, obj, cls):\r\n        return property.__get__(self, cls, cls)\r\n\r\n    def __set__(self, obj, value):\r\n        cls = obj if isinstance(obj, type) else type(obj)\r\n        property.__set__(self, cls, value)\r\n```\r\n\r\nThe `static_property` still needs a bit of support from a metaclass (see the code for details), but this is only a uniform behavior change and the metaclass doesn't need to store any data. Therefore, the metaclass doesn't need to be unique per type. We create a single one and share it among all the types which need it. And since that's all there is, we can also just let all types point to it and have \"free\" static properties.\r\n\r\nThis resolves one part of the multiple inheritance MRO issues: having a common metaclass for all types makes them compatible while the unique metatypes were blockers. However, there is an additional hurdle. In order to fully satisfy Python's type layout requirements, all types should have a common 'solid' base. A solid base is one which has the same instance size as the derived type (not counting the space required for the optional `dict_ptr` and `weakrefs_ptr`). Thus, `object` does not qualify as a solid base for pybind11 types and this can lead to issues with multiple inheritance.\r\n```\r\n             object\r\n            /      \\\r\nPybind11Type1      Pybind11Type2\r\n            \\      /\r\n             MIType\r\n```\r\nIn the example above, `instance_size(Pybind11Type1) > instance_size(object)` and `instance_size(Pybind11Type1) == instance_size(Pybind11Type2)` so `object` is not considered a solid base of `MIType`. This can result in `PyType_Ready()` complaining about layout and refusing to finalize the type for some base combinations.\r\n\r\nThe solution is to create a new base type:\r\n```\r\n         pybind11_object\r\n            /      \\\r\nPybind11Type1      Pybind11Type2\r\n            \\      /\r\n             MIType\r\n```\r\nHere `instance_size(pybind11_object) == instance_size(Pybind11Type1, Pybind11Type2)` and thus Python is perfectly happy to support it, even if the type is defined in Python itself:\r\n```\r\nclass MIType(Pybind11Type1, Pybind11Type2):\r\n    \"\"\"Inheritance works in Python\"\"\"\r\n```\r\n\r\n### Notes\r\n\r\nSince this changes a good bit of the `py::class_` implementation, I've also used this opportunity to move most of the details into a separate file (`detail/class_.h`) since the main `pybind11.h` is loaded enough as it is.","created_at":"2017-02-17T14:22:22Z","updated_at":"2017-02-20T15:44:30Z","closed_at":null,"merged_at":null,"merge_commit_sha":"764300cbdde5ffd5243d2bdebbadca073a394d30","assignee":null,"assignees":[],"milestone":null,"commits_url":"https://api.github.com/repos/pybind/pybind11/pulls/679/commits","review_comments_url":"https://api.github.com/repos/pybind/pybind11/pulls/679/comments","review_comment_url":"https://api.github.com/repos/pybind/pybind11/pulls/comments{/number}","comments_url":"https://api.github.com/repos/pybind/pybind11/issues/679/comments","statuses_url":"https://api.github.com/repos/pybind/pybind11/statuses/69a91ab869b54d2b150ee92d3e603f7dce4fcb20","head":{"label":"dean0x7d:mi-metaclass","ref":"mi-metaclass","sha":"69a91ab869b54d2b150ee92d3e603f7dce4fcb20","user":{"login":"dean0x7d","id":4831417,"avatar_url":"https://avatars.githubusercontent.com/u/4831417?v=3","gravatar_id":"","url":"https://api.github.com/users/dean0x7d","html_url":"https://github.com/dean0x7d","followers_url":"https://api.github.com/users/dean0x7d/followers","following_url":"https://api.github.com/users/dean0x7d/following{/other_user}","gists_url":"https://api.github.com/users/dean0x7d/gists{/gist_id}","starred_url":"https://api.github.com/users/dean0x7d/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/dean0x7d/subscriptions","organizations_url":"https://api.github.com/users/dean0x7d/orgs","repos_url":"https://api.github.com/users/dean0x7d/repos","events_url":"https://api.github.com/users/dean0x7d/events{/privacy}","received_events_url":"https://api.github.com/users/dean0x7d/received_events","type":"User","site_admin":false},"repo":{"id":59426490,"name":"pybind11","full_name":"dean0x7d/pybind11","owner":{"login":"dean0x7d","id":4831417,"avatar_url":"https://avatars.githubusercontent.com/u/4831417?v=3","gravatar_id":"","url":"https://api.github.com/users/dean0x7d","html_url":"https://github.com/dean0x7d","followers_url":"https://api.github.com/users/dean0x7d/followers","following_url":"https://api.github.com/users/dean0x7d/following{/other_user}","gists_url":"https://api.github.com/users/dean0x7d/gists{/gist_id}","starred_url":"https://api.github.com/users/dean0x7d/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/dean0x7d/subscriptions","organizations_url":"https://api.github.com/users/dean0x7d/orgs","repos_url":"https://api.github.com/users/dean0x7d/repos","events_url":"https://api.github.com/users/dean0x7d/events{/privacy}","received_events_url":"https://api.github.com/users/dean0x7d/received_events","type":"User","site_admin":false},"private":false,"html_url":"https://github.com/dean0x7d/pybind11","description":"Seamless operability between C++11 and Python","fork":true,"url":"https://api.github.com/repos/dean0x7d/pybind11","forks_url":"https://api.github.com/repos/dean0x7d/pybind11/forks","keys_url":"https://api.github.com/repos/dean0x7d/pybind11/keys{/key_id}","collaborators_url":"https://api.github.com/repos/dean0x7d/pybind11/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/dean0x7d/pybind11/teams","hooks_url":"https://api.github.com/repos/dean0x7d/pybind11/hooks","issue_events_url":"https://api.github.com/repos/dean0x7d/pybind11/issues/events{/number}","events_url":"https://api.github.com/repos/dean0x7d/pybind11/events","assignees_url":"https://api.github.com/repos/dean0x7d/pybind11/assignees{/user}","branches_url":"https://api.github.com/repos/dean0x7d/pybind11/branches{/branch}","tags_url":"https://api.github.com/repos/dean0x7d/pybind11/tags","blobs_url":"https://api.github.com/repos/dean0x7d/pybind11/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/dean0x7d/pybind11/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/dean0x7d/pybind11/git/refs{/sha}","trees_url":"https://api.github.com/repos/dean0x7d/pybind11/git/trees{/sha}","statuses_url":"https://api.github.com/repos/dean0x7d/pybind11/statuses/{sha}","languages_url":"https://api.github.com/repos/dean0x7d/pybind11/languages","stargazers_url":"https://api.github.com/repos/dean0x7d/pybind11/stargazers","contributors_url":"https://api.github.com/repos/dean0x7d/pybind11/contributors","subscribers_url":"https://api.github.com/repos/dean0x7d/pybind11/subscribers","subscription_url":"https://api.github.com/repos/dean0x7d/pybind11/subscription","commits_url":"https://api.github.com/repos/dean0x7d/pybind11/commits{/sha}","git_commits_url":"https://api.github.com/repos/dean0x7d/pybind11/git/commits{/sha}","comments_url":"https://api.github.com/repos/dean0x7d/pybind11/comments{/number}","issue_comment_url":"https://api.github.com/repos/dean0x7d/pybind11/issues/comments{/number}","contents_url":"https://api.github.com/repos/dean0x7d/pybind11/contents/{+path}","compare_url":"https://api.github.com/repos/dean0x7d/pybind11/compare/{base}...{head}","merges_url":"https://api.github.com/repos/dean0x7d/pybind11/merges","archive_url":"https://api.github.com/repos/dean0x7d/pybind11/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/dean0x7d/pybind11/downloads","issues_url":"https://api.github.com/repos/dean0x7d/pybind11/issues{/number}","pulls_url":"https://api.github.com/repos/dean0x7d/pybind11/pulls{/number}","milestones_url":"https://api.github.com/repos/dean0x7d/pybind11/milestones{/number}","notifications_url":"https://api.github.com/repos/dean0x7d/pybind11/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/dean0x7d/pybind11/labels{/name}","releases_url":"https://api.github.com/repos/dean0x7d/pybind11/releases{/id}","deployments_url":"https://api.github.com/repos/dean0x7d/pybind11/deployments","created_at":"2016-05-22T17:59:20Z","updated_at":"2017-02-07T21:31:41Z","pushed_at":"2017-02-20T15:43:39Z","git_url":"git://github.com/dean0x7d/pybind11.git","ssh_url":"git@github.com:dean0x7d/pybind11.git","clone_url":"https://github.com/dean0x7d/pybind11.git","svn_url":"https://github.com/dean0x7d/pybind11","homepage":null,"size":2264,"stargazers_count":0,"watchers_count":0,"language":"C++","has_issues":false,"has_downloads":true,"has_wiki":false,"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":"pybind:master","ref":"master","sha":"c8e506961ca205bdacf748a9be853300ef90af4d","user":{"login":"pybind","id":17565521,"avatar_url":"https://avatars.githubusercontent.com/u/17565521?v=3","gravatar_id":"","url":"https://api.github.com/users/pybind","html_url":"https://github.com/pybind","followers_url":"https://api.github.com/users/pybind/followers","following_url":"https://api.github.com/users/pybind/following{/other_user}","gists_url":"https://api.github.com/users/pybind/gists{/gist_id}","starred_url":"https://api.github.com/users/pybind/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/pybind/subscriptions","organizations_url":"https://api.github.com/users/pybind/orgs","repos_url":"https://api.github.com/users/pybind/repos","events_url":"https://api.github.com/users/pybind/events{/privacy}","received_events_url":"https://api.github.com/users/pybind/received_events","type":"Organization","site_admin":false},"repo":{"id":38581626,"name":"pybind11","full_name":"pybind/pybind11","owner":{"login":"pybind","id":17565521,"avatar_url":"https://avatars.githubusercontent.com/u/17565521?v=3","gravatar_id":"","url":"https://api.github.com/users/pybind","html_url":"https://github.com/pybind","followers_url":"https://api.github.com/users/pybind/followers","following_url":"https://api.github.com/users/pybind/following{/other_user}","gists_url":"https://api.github.com/users/pybind/gists{/gist_id}","starred_url":"https://api.github.com/users/pybind/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/pybind/subscriptions","organizations_url":"https://api.github.com/users/pybind/orgs","repos_url":"https://api.github.com/users/pybind/repos","events_url":"https://api.github.com/users/pybind/events{/privacy}","received_events_url":"https://api.github.com/users/pybind/received_events","type":"Organization","site_admin":false},"private":false,"html_url":"https://github.com/pybind/pybind11","description":"Seamless operability between C++11 and Python","fork":false,"url":"https://api.github.com/repos/pybind/pybind11","forks_url":"https://api.github.com/repos/pybind/pybind11/forks","keys_url":"https://api.github.com/repos/pybind/pybind11/keys{/key_id}","collaborators_url":"https://api.github.com/repos/pybind/pybind11/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/pybind/pybind11/teams","hooks_url":"https://api.github.com/repos/pybind/pybind11/hooks","issue_events_url":"https://api.github.com/repos/pybind/pybind11/issues/events{/number}","events_url":"https://api.github.com/repos/pybind/pybind11/events","assignees_url":"https://api.github.com/repos/pybind/pybind11/assignees{/user}","branches_url":"https://api.github.com/repos/pybind/pybind11/branches{/branch}","tags_url":"https://api.github.com/repos/pybind/pybind11/tags","blobs_url":"https://api.github.com/repos/pybind/pybind11/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/pybind/pybind11/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/pybind/pybind11/git/refs{/sha}","trees_url":"https://api.github.com/repos/pybind/pybind11/git/trees{/sha}","statuses_url":"https://api.github.com/repos/pybind/pybind11/statuses/{sha}","languages_url":"https://api.github.com/repos/pybind/pybind11/languages","stargazers_url":"https://api.github.com/repos/pybind/pybind11/stargazers","contributors_url":"https://api.github.com/repos/pybind/pybind11/contributors","subscribers_url":"https://api.github.com/repos/pybind/pybind11/subscribers","subscription_url":"https://api.github.com/repos/pybind/pybind11/subscription","commits_url":"https://api.github.com/repos/pybind/pybind11/commits{/sha}","git_commits_url":"https://api.github.com/repos/pybind/pybind11/git/commits{/sha}","comments_url":"https://api.github.com/repos/pybind/pybind11/comments{/number}","issue_comment_url":"https://api.github.com/repos/pybind/pybind11/issues/comments{/number}","contents_url":"https://api.github.com/repos/pybind/pybind11/contents/{+path}","compare_url":"https://api.github.com/repos/pybind/pybind11/compare/{base}...{head}","merges_url":"https://api.github.com/repos/pybind/pybind11/merges","archive_url":"https://api.github.com/repos/pybind/pybind11/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/pybind/pybind11/downloads","issues_url":"https://api.github.com/repos/pybind/pybind11/issues{/number}","pulls_url":"https://api.github.com/repos/pybind/pybind11/pulls{/number}","milestones_url":"https://api.github.com/repos/pybind/pybind11/milestones{/number}","notifications_url":"https://api.github.com/repos/pybind/pybind11/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/pybind/pybind11/labels{/name}","releases_url":"https://api.github.com/repos/pybind/pybind11/releases{/id}","deployments_url":"https://api.github.com/repos/pybind/pybind11/deployments","created_at":"2015-07-05T19:46:48Z","updated_at":"2017-02-19T11:17:36Z","pushed_at":"2017-02-20T15:43:40Z","git_url":"git://github.com/pybind/pybind11.git","ssh_url":"git@github.com:pybind/pybind11.git","clone_url":"https://github.com/pybind/pybind11.git","svn_url":"https://github.com/pybind/pybind11","homepage":"","size":2264,"stargazers_count":1798,"watchers_count":1798,"language":"C++","has_issues":true,"has_downloads":true,"has_wiki":false,"has_pages":false,"forks_count":203,"mirror_url":null,"open_issues_count":36,"forks":203,"open_issues":36,"watchers":1798,"default_branch":"master"}},"_links":{"self":{"href":"https://api.github.com/repos/pybind/pybind11/pulls/679"},"html":{"href":"https://github.com/pybind/pybind11/pull/679"},"issue":{"href":"https://api.github.com/repos/pybind/pybind11/issues/679"},"comments":{"href":"https://api.github.com/repos/pybind/pybind11/issues/679/comments"},"review_comments":{"href":"https://api.github.com/repos/pybind/pybind11/pulls/679/comments"},"review_comment":{"href":"https://api.github.com/repos/pybind/pybind11/pulls/comments{/number}"},"commits":{"href":"https://api.github.com/repos/pybind/pybind11/pulls/679/commits"},"statuses":{"href":"https://api.github.com/repos/pybind/pybind11/statuses/69a91ab869b54d2b150ee92d3e603f7dce4fcb20"}}}}
{"actor":{"display_login":"dean0x7d"}}