-
Notifications
You must be signed in to change notification settings - Fork 19
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Dynamically allocate char buffers when converting from Matlab strings #89
base: matlab
Are you sure you want to change the base?
Dynamically allocate char buffers when converting from Matlab strings #89
Conversation
@VladimirIvan @jaeandersson What do you think to have a test that check this improvement? A new test in swig/Examples/test-suite/matlab/ for example? |
I'm not familiar with this test setup but I added code which should (to my best knowledge) test this feature. |
There are several issues with this pull request:
|
@jaeandersson To answer to your concerns:
You are right, the function malloc/free should be used instead
That's true too. We could have a static array (as in the current code) and in case the string is greater than 255 characters, we could use a dynamic array.
The lines 238 and 245 in the new code (i.e. |
That can be done as a separate pull request to optimize this behaviour if you believe it will improve performance. I wouldn't suggest it without profiling though. Adding a condition may also slow down the evaluation but with compiler optimizations on, chances are none of these would matter anyway. Besides, Matlab code calling any function wrapped like this will most probably be the bottleneck, not the memory allocation of a string. If this makes any difference to anyone's code they probably shouldn't be writing their code in Matlab in the first place. |
I do not see the use of the
|
My bad. I didn't fully read the context in which the function is implemented. I have pushed the changes you suggested. |
There is still a memory leak. If "alloc" is false, where will the buffer be deallocated? |
I see the problem. In case
In the Python 3 bindings, the conversion will fail in case I propose to redesign the code as following: /* WARNING not tested */
size_t len=mxGetM(pm) * mxGetN(pm) + 1;
if (cptr) {
if (alloc) {
*cptr = %new_array(char,len);
int flag = mxGetString(pm,*cptr,len);
if (flag) {
%delete_array(*cptr);
return SWIG_TypeError;
}
*alloc = SWIG_NEWOBJ;
} else {
/* We can't allow converting without allocation, since we cannot access directly to the internal representation of the data in Matlab. */
return SWIG_RuntimeError;
}
}
if (psize) *psize = len;
return SWIG_OK; This code also also reduces the number of memory allocation needed compared to the current PR. |
This PR fixes issue #88