Do not wrap NULL pointer in array #264
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #264 +/- ##
=======================================
Coverage 99.87% 99.87%
=======================================
Files 2 2
Lines 784 787 +3
=======================================
+ Hits 783 786 +3
Misses 1 1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
src/NLopt.jl
Outdated
| d = unsafe_pointer_to_objref(d_)::Callback_Data | ||
| x = unsafe_wrap(Array, p_x, (n,)) | ||
| grad = unsafe_wrap(Array, p_grad, (n,)) | ||
| grad = p_grad == C_NULL ? Cdouble[] : unsafe_wrap(Array, p_grad, (n,)) |
There was a problem hiding this comment.
Should we declare a const empty_grad = Cdouble[] and just re-use it, since the caller shouldn't modify it?
There was a problem hiding this comment.
The matrix case below should be definitely safe. However, for the vector case the user in principle could resize the array. I've added a check to make sure the user doesn't resize the array when using it, which should be very cheap.
The minor issue with this is that the error will be thrown on the next use of the callback if the user modified the gradient array. Hopefully that's not a big problem. In the pre-memory version of julia, there used to be a way to make an vector non-resizable but I don't think that's possible on the latest version anymore.
|
I'll fix the formatting from #265 in a separate PR. |
| const _empty_vector = Cdouble[] | ||
|
|
||
| @inline function _get_empty_vector() | ||
| @assert isempty(_empty_vector) "Builtin empty vector modified by user" |
There was a problem hiding this comment.
Seems like it should have been a regular runtime check and exception rather than @assert, since we can't guarantee that this will always be true?
There was a problem hiding this comment.
I was initially thinking about it but felt like the most appropriate error to throw is AssertionError. I then realized that this can kind of be thought of as an internal consistency check if we claim that the user should not resize the array.
There was a problem hiding this comment.
That said, I'm fine with either way. If the grad input is supposed to be something that is undefined if modified by the user, then assert should be fine. If the grad is something that the user can mess with to get a well defined error then there should be a more meaningful error and also probably have a test for that. I slightly prefer the undefined behavior version since that's the only possible way in a multithreaded context.
3 participants
Just as an optimization. I don't think in practice this will cause any crashes but it saves one allocation when it's unnecessary and it's being done for the other callback below already.