Fix lifetime warning in Rust 1.70 #352
Conversation
| CString::new((*fi.next().unwrap()).clone()) | ||
| .unwrap() | ||
| .as_ptr() | ||
| fi.next().unwrap().as_ptr() |
There was a problem hiding this comment.
Wouldn't we have the same problem as before? the next will return the CString which will be dropped right away and you will return its pointer. What am I missing?
There was a problem hiding this comment.
I think we should just get rid of that macro for good if we can, and replace it with a function. I already fixed it in the past but then reverted for some reason (I can't remember), or we probably had another place with a similar issue.
There was a problem hiding this comment.
I do not think that the macro is the problem, working with pointer is unsafe and should be done carefully. Regardless, I do not like this macro as well ...
There was a problem hiding this comment.
I don't think the macro itself is a problem as, well. I think the way it is written (and used) is.
There was a problem hiding this comment.
We should probably add a proc macro on redismodule-rs-macros that will generated this call and this way we will also not be limited to the number of fields...
There was a problem hiding this comment.
Sounds like a good idea to try.
There was a problem hiding this comment.
I did some testing to check that the destructor is called at the correct time, but on second (third? fourth?) thought that testing might have been flawed and you might be right about the lifetime still being wrong.
There was a problem hiding this comment.
Not sure I understand how a proc macro would work, but it's been a while since I've dug into the details of Rust macros. I'm all in favor of it if it's possible.
There was a problem hiding this comment.
I suggest we will fix the current issue first, and open another issue to define this proc macro.
Starting with rustc 1.70.0, the code in raw.rs:hash_get_multi produced the
following warnings:
this `CString` is deallocated at the end of the statement, bind it to a
variable to extend its lifetime
While it would work in practice due to the underlying implementation just
doing pointer juggling, this warning was correct: The CString created within
the macro `f` would be destroyed before the reference to the pointer taken
from it with `as_ptr` was used.
Resolved by binding the lifetime of the CStrings to that of the iterator
variable, which is alive until the function returns.
|
Pushed a new commit that should properly resolve this by (regrettably) allocating a new Unfortunately I don't have any easy way of testing this actually works. Would appreciate some help on that front. |
There was a problem hiding this comment.
LGTM
I was wondering how come our valgrind run did not catch it, but then I notice we do not have such run :) (seems like I already open an issue to add it), so till then I believe we can simply make sure that the warning is gone.
I believe we should also open a followup issue to design this API with proc macros (as discussed here).
Starting with rustc 1.70.0, the code in raw.rs:hash_get_multi produced the following warnings:
While it would work in practice due to the underlying implementation just doing pointer juggling, this warning was correct: The CString created within the macro
fwould be destroyed before the reference to the pointer taken from it withas_ptrwas used.Resolved by allocating a new Vec that contains the CStrings. This ensures they remain alive until the function returns.