update(libsinsp): get_dynamic_field constness#2511
update(libsinsp): get_dynamic_field constness#2511deepskyblue86 wants to merge 1 commit intofalcosecurity:masterfrom
Conversation
Signed-off-by: Angelo Puglisi <angelopuglisi86@gmail.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: deepskyblue86 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2511 +/- ##
=======================================
Coverage 78.35% 78.35%
=======================================
Files 281 281
Lines 31689 31689
Branches 4643 4643
=======================================
+ Hits 24830 24831 +1
+ Misses 6859 6858 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
/milestone 0.22.0 |
| } | ||
|
|
||
| std::vector<void*> m_fields; | ||
| mutable std::vector<void*> m_fields; |
There was a problem hiding this comment.
What's the reason behind this mutable here?
There was a problem hiding this comment.
We return a pointer to an item inside m_fields, but if the vector is too small, we resize it first (see _access_dynamic_field() above)
There was a problem hiding this comment.
get_dynamic_field calls _access_dynamic_field.
Dynamic fields are lazily constructed there, and inserted into m_fields.
Without the mutable we can't make the thing const.
There was a problem hiding this comment.
What about providing a separate const accessor to the field? In general, I would avoid using mutable as I think it is just a way of hiding non-constness.
There was a problem hiding this comment.
That won't work, unless we make the field loading non-lazy, and I assume it was done like that on purpose (performance reasons? avoiding allocations until the field gets accessed?).
If you think of it as an internal cache, we're not really breaking the "OOP" constness of the class.
There was a problem hiding this comment.
Can we e.g. return nullptr from the const accessor (and handle it in a higher layer by returning the default value)?
This would make it even lazier, if anything (the field would get allocated on write, not on read). I have a vague uneasy feeling about mutable (something something thread safety), but if the nullptr approach doesn't work for whatever reason, I guess this is the way to go
|
/milestone 0.23.0 |
6 participants
What type of PR is this?
Any specific area of the project related to this PR?
/area libsinsp
Does this PR require a change in the driver versions?
What this PR does / why we need it:
Have get_dynamic_field being const, so it can be used as a regular getter.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?: