Memset should not be used for struct #4589
Conversation
Use aggregate {} initialization instead of low-level memset
for better C++ idiom compliance and clearer intent.
Signed-off-by: Diego Ostuni <dg.ostuni@gmail.com>
The XEvent and Screen members already use in-class member initializers. Removing the manual assignments in the constructor body to avoid redundant operations and simplify the code. Signed-off-by: Diego Ostuni <dg.ostuni@gmail.com>
tarek-y-ismail
left a comment
tarek-y-ismail
left a comment
There was a problem hiding this comment.
Thanks for the fixes. Just one small change needed.
| global_mock = this; | ||
|
|
||
| memset(&empty_object_props, 0, sizeof(empty_object_props)); | ||
| empty_object_props = {}; |
There was a problem hiding this comment.
This line can probably be entirely removed in favor of updating the member declaration to:
drmModeObjectProperties empty_object_props{};
similar to what you did with x11_mock.{h,cpp}
There was a problem hiding this comment.
Pull request overview
This PR replaces C-style memset calls with modern C++ struct initialization ({}) to follow more idiomatic C++ practices. The changes improve code safety by ensuring all struct members are properly zero-initialized through language features rather than manual memory manipulation.
Key Changes:
- Replaced
memsetwith aggregate initialization ({}) for struct zero-initialization - Moved member initialization from constructor body to in-class member initializers in
FakeX11Resources
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
tests/unit-tests/platforms/gbm-kms/kms-utils/test_drm_mode_resources.cpp |
Replaced memset with = {} for drmModePropertyRes struct |
tests/mir_test_doubles/mock_x11.cpp |
Removed redundant memset calls from constructor (initialization moved to header) |
tests/mir_test_doubles/mock_drm.cpp |
Replaced memset with = {} for empty_object_props struct |
tests/include/mir/test/doubles/mock_x11.h |
Changed in-class initializers from = { 0 } to = {} for consistency |
src/platforms/gbm-kms/server/kms/kms_page_flipper.cpp |
Replaced memset with {} initialization for drmEventContext struct |
src/platforms/common/server/kms-utils/threaded_drm_event_handler.cpp |
Replaced memset with {} initialization for drmEventContext struct |
src/common/dispatch/multiplexing_dispatchable.cpp |
Replaced memset with {} initialization for epoll_event struct |
AlanGriffiths
left a comment
AlanGriffiths
left a comment
There was a problem hiding this comment.
I think the mess in test_drm_mode_resources.cpp is best considered "out of scope" here: It requires a more complex fix. Just leave it alone for now
| auto const& property = properties.at(id); | ||
|
|
||
| auto prop = | ||
| std::unique_ptr<drmModePropertyRes, void (*)(drmModePropertyPtr)>(new drmModePropertyRes, &::drmModeFreeProperty); |
There was a problem hiding this comment.
[pre-existing] this code looks weird: we are allocating with new but potentially freeing with drmModeFreeProperty (which is unlikely to be a synonym for delete).
| std::unique_ptr<drmModePropertyRes, void (*)(drmModePropertyPtr)>(new drmModePropertyRes, &::drmModeFreeProperty); | ||
|
|
||
| memset(prop.get(), 0, sizeof(*prop)); | ||
| *prop = {}; |
There was a problem hiding this comment.
I think the initialization should be done in the new expression.
3 participants
Closes #4564
Related: #4564
What's new?
How to test
Checklist