Copy attachments buffer when parsing summary [20078] #100
Conversation
juanlofer-eprosima
temporarily deployed
to
codecov
GitHub Actions
Inactive
juanlofer-eprosima
changed the title
Signed-off-by: Juan Lopez Fernandez <juanlopez@eprosima.com>
juanlofer-eprosima
force-pushed
the
hotfix/copy-attachments-buffer
branch
from
7eb361a to
c941084
Compare
juanlofer-eprosima
temporarily deployed
to
codecov
GitHub Actions
Inactive
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #100 +/- ##
==========================================
- Coverage 20.36% 20.19% -0.18%
==========================================
Files 29 29
Lines 2715 2714 -1
Branches 1423 1423
==========================================
- Hits 553 548 -5
- Misses 1694 1704 +10
+ Partials 468 462 -6 ☔ View full report in Codecov by Sentry. |
thirdparty/mcap/mcap/reader.inl
Outdated
| Attachment attachment_copy = attachment; | ||
| attachment_copy.data = (std::byte*)std::malloc(attachment.dataSize); | ||
| std::memcpy((void*)attachment_copy.data, attachment.data, attachment.dataSize); | ||
| attachments_.emplace(attachment_copy.name, attachment_copy); |
There was a problem hiding this comment.
Consider having std::move(attachment_copy) to avoid copying the attachment_copy unnecessarily when emplicing it in the attachment_.
| // Copy buffer for storage as original is freed when leaving scope | ||
| Attachment attachment_copy = attachment; | ||
| attachment_copy.data = (std::byte*)std::malloc(attachment.dataSize); | ||
| std::memcpy((void*)attachment_copy.data, attachment.data, attachment.dataSize); |
There was a problem hiding this comment.
Is the (void *) casting necessary?
There was a problem hiding this comment.
I believe that is the case, I suppose I added it for a reason.
There was a problem hiding this comment.
You're right. It's required.
| } | ||
| attachments_.clear(); | ||
| } | ||
|
|
There was a problem hiding this comment.
The function .clear() calls the destructor of each element inside the map. Wouldn't it make more sense to override the destructor of the Attachment object to free attachment.data? In that way you would avoid having a clear_attachments_ function and simply call attachments_.clear() like with every other object.
There was a problem hiding this comment.
I don't think that would be valid, because elsewhere (every place other than when destroying our added map) it would be calling a free that is already called somewhere else (there is no way of checking if already freed easily, perhaps adding an attribute, but too involved and risky).
Another option would be to inherit from Attachment and add that destructor only there (and have a map of children). I would not go for that, our changes to the mcap library should be minimal.
Signed-off-by: Juan Lopez Fernandez <juanlopez@eprosima.com>
juanlofer-eprosima
temporarily deployed
to
codecov
GitHub Actions
Inactive
juanlofer-eprosima
temporarily deployed
to
codecov
GitHub Actions
Inactive
MCAP C++ library was previously modified so that attachments (and metadata) are stored in a class attribute, accessible through public methods, while parsing a file. However, attachments were not well stored as their internal buffer was not being copied (only their address), and after parsing this memory could be deallocated or reused in posterior actions.
Related: