Fixes couple of native memory leaks#1597
Conversation
There was a problem hiding this comment.
most of the deletion should be delete and not delete[].
Rule being:
- Allocate using
new, delete withdelete - Allocate using
new[]delete withdelete[]
The mixup is considered as undefined behavior
Maybe someday, as a big refactoring moving to std::unique_ptr/std::shared_ptr (depending on the semantic) could be a better option.
| if (find_res != modules.end()) | ||
| { | ||
| auto moduleHandler = find_res->second; | ||
| delete[] moduleHandler; |
There was a problem hiding this comment.
shoudn't it be delete moduleHandler instead ?
| this->module = nullptr; | ||
| if (this->functionInfo != nullptr) | ||
| { | ||
| delete[] this->functionInfo; |
There was a problem hiding this comment.
shouldn't it be delete this->functionInfo instead ?
There was a problem hiding this comment.
It's possible to use std::unique_ptr here instead.
| this->handler = nullptr; | ||
| for (auto moduleMethod : methods) | ||
| { | ||
| delete[] moduleMethod.second; |
There was a problem hiding this comment.
should'nt it be delete moduleMethod.second instead?
There was a problem hiding this comment.
for methods it's also possible to use std::unique_ptr
| { | ||
| if (this->rejit_queue_ != nullptr) | ||
| { | ||
| delete[] this->rejit_queue_; |
There was a problem hiding this comment.
shoudn't it be delete this->rejit_queue_; instead?
| } | ||
| if (this->rejit_queue_thread_ != nullptr) | ||
| { | ||
| delete[] this->rejit_queue_thread_; |
There was a problem hiding this comment.
shouldn't it bedelete this->rejit_queue_thread_; instead ?
| if (rejit_handler != nullptr) | ||
| { | ||
| rejit_handler->Shutdown(); | ||
| delete[] rejit_handler; |
There was a problem hiding this comment.
should'nt it be delete rejit_handler; instead ?
| { | ||
| if (calltargetTokens != nullptr) | ||
| { | ||
| delete[] calltargetTokens; |
There was a problem hiding this comment.
shouldn't it be delete calltargetTokens instead ?
There was a problem hiding this comment.
I think in this case you can go for a std::unique_ptr.
|
|
||
| hr = rewriter.Export(); | ||
|
|
||
| delete[] newEHClauses; |
There was a problem hiding this comment.
That could be risky no? you pass its pointer to the rewriter and ends it lifecyle before the rewriter lifecyle.
Couldn't you use std::unique_ptr instead ? (and passing the pointer to the rewriter using the .get method )?
Yes is a WIP, I had to go out before completing this. That’s why the WIP label and the Draft 😊 |
| this->rejit_queue_ = new BlockingQueue<RejitItem>(); | ||
| this->rejit_queue_thread_ = new std::thread(enqueue_thread, this); | ||
| } | ||
| ~RejitHandler() |
There was a problem hiding this comment.
for both, it's possible to use std::unique_ptr instead
|
I tested a bit and overall, you can replace every raw pointers by a std::unique_ptr ;) |
|
Superseeded by #1600 |
This PR is a continuation of #1596 handling a couple of possible memory leaks in the native library.
@DataDog/apm-dotnet