{"action":"created","comment":{"url":"https://api.github.com/repos/sympy/symengine/pulls/comments/33674272","id":33674272,"diff_hunk":"@@ -200,4 +210,78 @@ int is_a_Symbol(const basic c)\n     return is_a<Symbol>(*(*RCP_const_cast(c)));\n }\n \n+\n+// C wrapper for std::vector<int>\n+\n+struct CVectorInt {\n+    std::vector<int> m;\n+};\n+\n+CVectorInt* vectorint_new()\n+{\n+    return new CVectorInt;\n+}\n+\n+int vectorint_placement_new(CVectorInt *self, size_t size)\n+{\n+    if (!SymEngine::is_aligned(self)) return 2;\n+    if (size < sizeof(CVectorInt)) return 1;\n+    new(self) CVectorInt;\n+    return 0;\n+}\n+\n+void vectorint_free(CVectorInt *self)\n+{\n+    delete self;\n+}","path":"symengine/cwrapper.cpp","position":44,"original_position":44,"commit_id":"776e706337d51d48b14e5520ef42160d610c41e1","original_commit_id":"776e706337d51d48b14e5520ef42160d610c41e1","user":{"login":"certik","id":20568,"avatar_url":"https://avatars.githubusercontent.com/u/20568?v=3","gravatar_id":"","url":"https://api.github.com/users/certik","html_url":"https://github.com/certik","followers_url":"https://api.github.com/users/certik/followers","following_url":"https://api.github.com/users/certik/following{/other_user}","gists_url":"https://api.github.com/users/certik/gists{/gist_id}","starred_url":"https://api.github.com/users/certik/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/certik/subscriptions","organizations_url":"https://api.github.com/users/certik/orgs","repos_url":"https://api.github.com/users/certik/repos","events_url":"https://api.github.com/users/certik/events{/privacy}","received_events_url":"https://api.github.com/users/certik/received_events","type":"User","site_admin":false},"body":"You should not call delete on it. The caller provided the memory, the\ncaller is responsible to free it. However, you are right that we need to\ncall the destructor ourselves, i.e. provide a function to do that. Perhaps\ncall it vectorint_placement_free or something like that, and it would just\ncall the destructor.\n\nSent from my mobile phone.\nOn Jul 1, 2015 6:37 AM, \"Abinash Meher\" <notifications@github.com> wrote:\n\n> In symengine/cwrapper.cpp\n> <https://github.com/sympy/symengine/pull/495#discussion_r33673666>:\n>\n> > +{\n> > +    return new CVectorInt;\n> > +}\n> > +\n> > +int vectorint_placement_new(CVectorInt *self, size_t size)\n> > +{\n> > +    if (!SymEngine::is_aligned(self)) return 2;\n> > +    if (size < sizeof(CVectorInt)) return 1;\n> > +    new(self) CVectorInt;\n> > +    return 0;\n> > +}\n> > +\n> > +void vectorint_free(CVectorInt *self)\n> > +{\n> > +    delete self;\n> > +}\n>\n> Let's say we have more memory than sizeof(CVectorInt) allocated on the\n> stack, and we use vector_placement_new to initialize CVectorInt at the\n> given location. So, when we call delete on the pointer to that location,\n> will the whole memory(that is more than the size of CVectorInt) be freed?\n> If I am guessing correctly, does SymEngine::is_aligned has anything to do\n> to ensure that?\n>\n> —\n> Reply to this email directly or view it on GitHub\n> <https://github.com/sympy/symengine/pull/495/files#r33673666>.\n>\n","created_at":"2015-07-01T12:46:31Z","updated_at":"2015-07-01T12:46:31Z","html_url":"https://github.com/sympy/symengine/pull/495#discussion_r33674272","pull_request_url":"https://api.github.com/repos/sympy/symengine/pulls/495","_links":{"self":{"href":"https://api.github.com/repos/sympy/symengine/pulls/comments/33674272"},"html":{"href":"https://github.com/sympy/symengine/pull/495#discussion_r33674272"},"pull_request":{"href":"https://api.github.com/repos/sympy/symengine/pulls/495"}}},"pull_request":{"url":"https://api.github.com/repos/sympy/symengine/pulls/495","id":38930975,"html_url":"https://github.com/sympy/symengine/pull/495","diff_url":"https://github.com/sympy/symengine/pull/495.diff","patch_url":"https://github.com/sympy/symengine/pull/495.patch","issue_url":"https://api.github.com/repos/sympy/symengine/issues/495","number":495,"state":"open","locked":false,"title":"Wrap vec_basic and get_args() into C","user":{"login":"certik","id":20568,"avatar_url":"https://avatars.githubusercontent.com/u/20568?v=3","gravatar_id":"","url":"https://api.github.com/users/certik","html_url":"https://github.com/certik","followers_url":"https://api.github.com/users/certik/followers","following_url":"https://api.github.com/users/certik/following{/other_user}","gists_url":"https://api.github.com/users/certik/gists{/gist_id}","starred_url":"https://api.github.com/users/certik/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/certik/subscriptions","organizations_url":"https://api.github.com/users/certik/orgs","repos_url":"https://api.github.com/users/certik/repos","events_url":"https://api.github.com/users/certik/events{/privacy}","received_events_url":"https://api.github.com/users/certik/received_events","type":"User","site_admin":false},"body":"This uses a clean approach to wrap C++ classes into C. No casting is done in this new code (except for the already implemented `basic` type itself, which we need to refactor to use this new approach later), so both the C and C++ compilers know all they need to know about the pointers:\r\n\r\n* From the C perspective, each type is an opaque structure (but it's not a void pointer, nor any other casts are necessary), so the C compiler can check the type\r\n* From the C++ perspective, the opaque type from the .h file is implemented in the .cpp file as a simple C structure which has the C++ data member `m` in it.\r\n\r\nThis should have no runtime overhead. You access the `m` member directly.\r\n\r\nThe ugly function that @abinashmeher999 and I were designing here: https://github.com/abinashmeher999/symengine/commit/b32926674800476176d23200439c7e66c9324b26#commitcomment-11936123:\r\n```c++\r\nvoid basic_get_args(const basic a, basic_struct **array, int *size)\r\n{\r\n    SymEngine::vec_basic args = (*RCP_const_cast(a))->get_args();\r\n    *size = args.size();\r\n    *array = (basic_struct*)malloc((*size)*sizeof(basic_struct));\r\n    int index = 0;\r\n    for (auto &a: args) {\r\n        *RCP_cast(&((*array)[index++])) = a;\r\n    }\r\n}\r\n```\r\nnow becomes a simple two liner:\r\n```c++\r\nvoid basic_get_args(const basic self, CVecBasic *args)\r\n{\r\n    for (auto &a: (*RCP_const_cast(self))->get_args())\r\n        args->m.push_back(a);\r\n}\r\n```\r\nThe only ugly cast comes from the `basic` type. After we upgrade the rest of the C wrappers, this function will look like this:\r\n```c++\r\nvoid basic_get_args(const CRCPBasic *self, CVecBasic *args)\r\n{\r\n    for (auto &a: (self->m->get_args())\r\n        args->m.push_back(a);\r\n}\r\n```\r\nWhere `CRCPBasic` is defined as:\r\n```c++\r\nstruct CVecBasic {\r\n    SymEngine::RCP<const SymEngine::Basic> m;\r\n};\r\n```\r\n\r\nSo everything becomes clean, no casts and as safe as it can be in C. Writing the C wrappers then just becomes writing a wrapper C structure with the C++ `m` member in it, and accessing and manipulating `m` directly using C++. The only potentially unsafe operation is dereferencing the function arguments (e.g. `const CRCPBasic *self`) to access the `m` member. Manipulating the `m` member is then done in C++ and thus is 100% safe in Debug mode. The only way this can fail is if the user doesn't allocate the memory (e.g. for `self`), but there is no way around that in C.\r\n\r\nThis PR also shows how to allocate directly on stack using placement new (see `test_CVectorInt2`):\r\n```c++\r\n    char data3[50]; // Aligned properly and enough size to fit std::vector<int>\r\n    CVectorInt *vec = (CVectorInt*)data3;\r\n    assert(vectorint_placement_new(vec, sizeof(data3)) == 0);\r\n```\r\nAnd that's how we need to allocate the `CRCPBasic` type. The approach of `vectorint_placement_new` is very general, since the C wrappers do not know its size, but for `CRCPBasic` we know its size, so we'll use the same trick that we already implemented in `basic_init`.\r\n\r\n/cc @isuruf, @thilinarmtb ","created_at":"2015-06-30T22:22:32Z","updated_at":"2015-07-01T12:46:32Z","closed_at":null,"merged_at":null,"merge_commit_sha":"337ca3de4c2a0c9fe4ba9979744feef6317e6c3b","assignee":null,"milestone":null,"commits_url":"https://api.github.com/repos/sympy/symengine/pulls/495/commits","review_comments_url":"https://api.github.com/repos/sympy/symengine/pulls/495/comments","review_comment_url":"https://api.github.com/repos/sympy/symengine/pulls/comments{/number}","comments_url":"https://api.github.com/repos/sympy/symengine/issues/495/comments","statuses_url":"https://api.github.com/repos/sympy/symengine/statuses/776e706337d51d48b14e5520ef42160d610c41e1","head":{"label":"certik:cwrappers_refactoring","ref":"cwrappers_refactoring","sha":"776e706337d51d48b14e5520ef42160d610c41e1","user":{"login":"certik","id":20568,"avatar_url":"https://avatars.githubusercontent.com/u/20568?v=3","gravatar_id":"","url":"https://api.github.com/users/certik","html_url":"https://github.com/certik","followers_url":"https://api.github.com/users/certik/followers","following_url":"https://api.github.com/users/certik/following{/other_user}","gists_url":"https://api.github.com/users/certik/gists{/gist_id}","starred_url":"https://api.github.com/users/certik/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/certik/subscriptions","organizations_url":"https://api.github.com/users/certik/orgs","repos_url":"https://api.github.com/users/certik/repos","events_url":"https://api.github.com/users/certik/events{/privacy}","received_events_url":"https://api.github.com/users/certik/received_events","type":"User","site_admin":false},"repo":{"id":18584165,"name":"symengine","full_name":"certik/symengine","owner":{"login":"certik","id":20568,"avatar_url":"https://avatars.githubusercontent.com/u/20568?v=3","gravatar_id":"","url":"https://api.github.com/users/certik","html_url":"https://github.com/certik","followers_url":"https://api.github.com/users/certik/followers","following_url":"https://api.github.com/users/certik/following{/other_user}","gists_url":"https://api.github.com/users/certik/gists{/gist_id}","starred_url":"https://api.github.com/users/certik/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/certik/subscriptions","organizations_url":"https://api.github.com/users/certik/orgs","repos_url":"https://api.github.com/users/certik/repos","events_url":"https://api.github.com/users/certik/events{/privacy}","received_events_url":"https://api.github.com/users/certik/received_events","type":"User","site_admin":false},"private":false,"html_url":"https://github.com/certik/symengine","description":"Fast symbolic manipulation library, written in C++","fork":true,"url":"https://api.github.com/repos/certik/symengine","forks_url":"https://api.github.com/repos/certik/symengine/forks","keys_url":"https://api.github.com/repos/certik/symengine/keys{/key_id}","collaborators_url":"https://api.github.com/repos/certik/symengine/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/certik/symengine/teams","hooks_url":"https://api.github.com/repos/certik/symengine/hooks","issue_events_url":"https://api.github.com/repos/certik/symengine/issues/events{/number}","events_url":"https://api.github.com/repos/certik/symengine/events","assignees_url":"https://api.github.com/repos/certik/symengine/assignees{/user}","branches_url":"https://api.github.com/repos/certik/symengine/branches{/branch}","tags_url":"https://api.github.com/repos/certik/symengine/tags","blobs_url":"https://api.github.com/repos/certik/symengine/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/certik/symengine/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/certik/symengine/git/refs{/sha}","trees_url":"https://api.github.com/repos/certik/symengine/git/trees{/sha}","statuses_url":"https://api.github.com/repos/certik/symengine/statuses/{sha}","languages_url":"https://api.github.com/repos/certik/symengine/languages","stargazers_url":"https://api.github.com/repos/certik/symengine/stargazers","contributors_url":"https://api.github.com/repos/certik/symengine/contributors","subscribers_url":"https://api.github.com/repos/certik/symengine/subscribers","subscription_url":"https://api.github.com/repos/certik/symengine/subscription","commits_url":"https://api.github.com/repos/certik/symengine/commits{/sha}","git_commits_url":"https://api.github.com/repos/certik/symengine/git/commits{/sha}","comments_url":"https://api.github.com/repos/certik/symengine/comments{/number}","issue_comment_url":"https://api.github.com/repos/certik/symengine/issues/comments{/number}","contents_url":"https://api.github.com/repos/certik/symengine/contents/{+path}","compare_url":"https://api.github.com/repos/certik/symengine/compare/{base}...{head}","merges_url":"https://api.github.com/repos/certik/symengine/merges","archive_url":"https://api.github.com/repos/certik/symengine/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/certik/symengine/downloads","issues_url":"https://api.github.com/repos/certik/symengine/issues{/number}","pulls_url":"https://api.github.com/repos/certik/symengine/pulls{/number}","milestones_url":"https://api.github.com/repos/certik/symengine/milestones{/number}","notifications_url":"https://api.github.com/repos/certik/symengine/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/certik/symengine/labels{/name}","releases_url":"https://api.github.com/repos/certik/symengine/releases{/id}","created_at":"2014-04-09T03:40:03Z","updated_at":"2015-06-10T11:28:21Z","pushed_at":"2015-06-30T22:42:40Z","git_url":"git://github.com/certik/symengine.git","ssh_url":"git@github.com:certik/symengine.git","clone_url":"https://github.com/certik/symengine.git","svn_url":"https://github.com/certik/symengine","homepage":"http://sympy.org","size":3517,"stargazers_count":3,"watchers_count":3,"language":"C++","has_issues":false,"has_downloads":true,"has_wiki":false,"has_pages":false,"forks_count":0,"mirror_url":null,"open_issues_count":1,"forks":0,"open_issues":1,"watchers":3,"default_branch":"master"}},"base":{"label":"sympy:master","ref":"master","sha":"bbe3c9baf653d18d37e3bfcd424e0781786098c1","user":{"login":"sympy","id":260832,"avatar_url":"https://avatars.githubusercontent.com/u/260832?v=3","gravatar_id":"","url":"https://api.github.com/users/sympy","html_url":"https://github.com/sympy","followers_url":"https://api.github.com/users/sympy/followers","following_url":"https://api.github.com/users/sympy/following{/other_user}","gists_url":"https://api.github.com/users/sympy/gists{/gist_id}","starred_url":"https://api.github.com/users/sympy/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/sympy/subscriptions","organizations_url":"https://api.github.com/users/sympy/orgs","repos_url":"https://api.github.com/users/sympy/repos","events_url":"https://api.github.com/users/sympy/events{/privacy}","received_events_url":"https://api.github.com/users/sympy/received_events","type":"Organization","site_admin":false},"repo":{"id":42376,"name":"symengine","full_name":"sympy/symengine","owner":{"login":"sympy","id":260832,"avatar_url":"https://avatars.githubusercontent.com/u/260832?v=3","gravatar_id":"","url":"https://api.github.com/users/sympy","html_url":"https://github.com/sympy","followers_url":"https://api.github.com/users/sympy/followers","following_url":"https://api.github.com/users/sympy/following{/other_user}","gists_url":"https://api.github.com/users/sympy/gists{/gist_id}","starred_url":"https://api.github.com/users/sympy/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/sympy/subscriptions","organizations_url":"https://api.github.com/users/sympy/orgs","repos_url":"https://api.github.com/users/sympy/repos","events_url":"https://api.github.com/users/sympy/events{/privacy}","received_events_url":"https://api.github.com/users/sympy/received_events","type":"Organization","site_admin":false},"private":false,"html_url":"https://github.com/sympy/symengine","description":"SymEngine is a fast symbolic manipulation library, written in C++ (SymEngine was previously called CSymPy)","fork":false,"url":"https://api.github.com/repos/sympy/symengine","forks_url":"https://api.github.com/repos/sympy/symengine/forks","keys_url":"https://api.github.com/repos/sympy/symengine/keys{/key_id}","collaborators_url":"https://api.github.com/repos/sympy/symengine/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/sympy/symengine/teams","hooks_url":"https://api.github.com/repos/sympy/symengine/hooks","issue_events_url":"https://api.github.com/repos/sympy/symengine/issues/events{/number}","events_url":"https://api.github.com/repos/sympy/symengine/events","assignees_url":"https://api.github.com/repos/sympy/symengine/assignees{/user}","branches_url":"https://api.github.com/repos/sympy/symengine/branches{/branch}","tags_url":"https://api.github.com/repos/sympy/symengine/tags","blobs_url":"https://api.github.com/repos/sympy/symengine/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/sympy/symengine/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/sympy/symengine/git/refs{/sha}","trees_url":"https://api.github.com/repos/sympy/symengine/git/trees{/sha}","statuses_url":"https://api.github.com/repos/sympy/symengine/statuses/{sha}","languages_url":"https://api.github.com/repos/sympy/symengine/languages","stargazers_url":"https://api.github.com/repos/sympy/symengine/stargazers","contributors_url":"https://api.github.com/repos/sympy/symengine/contributors","subscribers_url":"https://api.github.com/repos/sympy/symengine/subscribers","subscription_url":"https://api.github.com/repos/sympy/symengine/subscription","commits_url":"https://api.github.com/repos/sympy/symengine/commits{/sha}","git_commits_url":"https://api.github.com/repos/sympy/symengine/git/commits{/sha}","comments_url":"https://api.github.com/repos/sympy/symengine/comments{/number}","issue_comment_url":"https://api.github.com/repos/sympy/symengine/issues/comments{/number}","contents_url":"https://api.github.com/repos/sympy/symengine/contents/{+path}","compare_url":"https://api.github.com/repos/sympy/symengine/compare/{base}...{head}","merges_url":"https://api.github.com/repos/sympy/symengine/merges","archive_url":"https://api.github.com/repos/sympy/symengine/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/sympy/symengine/downloads","issues_url":"https://api.github.com/repos/sympy/symengine/issues{/number}","pulls_url":"https://api.github.com/repos/sympy/symengine/pulls{/number}","milestones_url":"https://api.github.com/repos/sympy/symengine/milestones{/number}","notifications_url":"https://api.github.com/repos/sympy/symengine/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/sympy/symengine/labels{/name}","releases_url":"https://api.github.com/repos/sympy/symengine/releases{/id}","created_at":"2008-08-13T18:40:14Z","updated_at":"2015-06-28T05:42:29Z","pushed_at":"2015-07-01T12:44:16Z","git_url":"git://github.com/sympy/symengine.git","ssh_url":"git@github.com:sympy/symengine.git","clone_url":"https://github.com/sympy/symengine.git","svn_url":"https://github.com/sympy/symengine","homepage":"http://sympy.org","size":22837,"stargazers_count":66,"watchers_count":66,"language":"C++","has_issues":true,"has_downloads":true,"has_wiki":true,"has_pages":false,"forks_count":58,"mirror_url":null,"open_issues_count":89,"forks":58,"open_issues":89,"watchers":66,"default_branch":"master"}},"_links":{"self":{"href":"https://api.github.com/repos/sympy/symengine/pulls/495"},"html":{"href":"https://github.com/sympy/symengine/pull/495"},"issue":{"href":"https://api.github.com/repos/sympy/symengine/issues/495"},"comments":{"href":"https://api.github.com/repos/sympy/symengine/issues/495/comments"},"review_comments":{"href":"https://api.github.com/repos/sympy/symengine/pulls/495/comments"},"review_comment":{"href":"https://api.github.com/repos/sympy/symengine/pulls/comments{/number}"},"commits":{"href":"https://api.github.com/repos/sympy/symengine/pulls/495/commits"},"statuses":{"href":"https://api.github.com/repos/sympy/symengine/statuses/776e706337d51d48b14e5520ef42160d610c41e1"}}}}