Dynamic memory audit #102
Dynamic memory audit #102
Conversation
wjwwood
added
enhancement
|
@wjwwood sorry but I can't recall it now - you said that there will be another PR against rcutils after this one? |
|
Yes, I will probably make it another one rather than combining it into this. |
wjwwood
force-pushed
the
dynamic_memory_audit
branch
from
266ef58
to
70baef0
Compare
wjwwood
added
in review
|
If anyone has time to review these, that would be appreciated! 👍 CI:
|
|
@serge-nikulin could you do it please? |
| @@ -1,42 +0,0 @@ | |||
| // Copyright 2017 Open Source Robotics Foundation, Inc. | |||
There was a problem hiding this comment.
why was this file removed?
I tried to find any mention but I couldn't.
There was a problem hiding this comment.
I just replaced all use of it with rcutils_format_string(), so it's unnecessary now.
| } | ||
| const size_t size = 1024; | ||
| g_last_log_event.message = malloc(size); | ||
| g_last_log_event.message = allocator.allocate(size, allocator.state); |
There was a problem hiding this comment.
so free resp. malloc are now called inside deallocate resp allocate?
wjwwood
added
in progress
|
Sorry, there are some missing, but related pull requests for this one. I'll fix it up and ask for review again. |
|
@wjwwood one more Q. I tried to find coverage information for rcutils but could only find this https://ci.ros2.org/job/ci_linux_coverage/2/cobertura/src_ros2_rcutils_src/ which seems a bit old. |
|
@serge-nikulin you're welcome to review it too, but I do need someone from our team to review it as well.
There's no coverage information still. We have what you linked to, but it's also not trusty worthy right now. |
wjwwood
added
in review
|
@dejanpan, when do you need the review? |
test/test_filesystem.cpp
Outdated
| @@ -19,8 +19,10 @@ | |||
|
|
|||
| static char cwd[1024]; | |||
|
|
|||
| static rcutils_allocator_t g_allocator = rcutils_get_default_allocator(); | |||
There was a problem hiding this comment.
Not actually changed in this patch but shouldn't all allocated return values be freed here?
There was a problem hiding this comment.
Yeah, looks like there are some memory leaks in that test, I'll clean them up.
There was a problem hiding this comment.
Yeah, those were really bad leaks. I think 717e044 fixed them.
|
Ok, in review again, new CI:
|
|
I could still use reviews of some of the other pr's associated with this pull request. New CI after fix up:
|
This pull request makes sure that we're using
rcutils_allocator_teverywhere and not usingmalloc,realloc,calloc, orfreeanywhere at all.Requires #101