-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Move data model types from basic-types.h to DataModelTypes.h #39146
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
I see some changes in the auto generated code (zzz_generated folder), just want to confirm if this change will not be overwritten by some tool like ZAP or whatever one is used to generate that code. |
src/app/clusters/boolean-state-configuration-server/boolean-state-configuration-server.h
Outdated
Show resolved
Hide resolved
@tersal Not really, the tests already build it again and test it, so if the tests were to fail when building then there'd be issues. That's not the case since we changed the BUILD.gn file which changes part of the code generation. |
d63c2a1
to
8ce15ad
Compare
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request effectively refactors common data model types by moving them from src/app/util/basic-types.h
to src/lib/core/DataModelTypes.h
. This centralization is a positive step for maintainability and clarity, placing these fundamental types in a more appropriate core location. The changes appear correct and maintain existing functionality for consumers of basic-types.h
.
One minor suggestion regarding the PR title: "Refactor data model types to include basic-types.h
" could be slightly ambiguous. Perhaps a title like "Move data model types from basic-types.h
to DataModelTypes.h
" or "Centralize data model types into DataModelTypes.h
" might more directly reflect the changes made. This is just a suggestion for clarity.
Overall, good work on this refactoring!
Summary of Findings
- Redundant include in
basic-types.h
: Insrc/app/util/basic-types.h
, the#include <cstdint>
(previously on line 26, now effectively line 26 in the full file context post-diff) appears redundant aslib/core/DataModelTypes.h
(included on line 29) already includes<cstdint>
. This was not added as a review comment due to severity settings. - Empty namespace in
basic-types.h
: The filesrc/app/util/basic-types.h
now contains an empty namespace declaration:namespace chip {} // namespace chip
(line 31 post-diff). This seems unnecessary if the file's primary role is now an include forwarder. This was not added as a review comment due to severity settings. - File Doxygen comment in
basic-types.h
: The Doxygen comment forsrc/app/util/basic-types.h
(lines 18-22 in the full file context) states it's 'The include file for all the types for the data model...'. Since the types are now defined inDataModelTypes.h
and only included here, this description might benefit from a slight update to reflect its current role. This was not added as a review comment due to severity settings.
Merge Readiness
The code changes are well-structured and achieve the goal of centralizing data model types. Based on the review criteria and severity settings, no medium or higher severity issues were found in the diff. The pull request appears to be in good shape for merging. As a reviewer, I am not authorized to approve pull requests, so please ensure it undergoes any further necessary review and approval processes.
basic-types.h
PR #39146: Size comparison from 49594a5 to 1524ff9 Increases above 0.2%:
Full report (75 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And followup (or here, either way, but followup is better): to address the rest of #38390 (comment), actually change the zapt to include DataModelTypes.h, right?
@@ -6,7 +6,7 @@ | |||
#include <app/data-model/DecodableList.h> | |||
#include <app/data-model/List.h> | |||
#include <app/data-model/Nullable.h> | |||
#include <app/util/basic-types.h> | |||
$include <lib/core/DataModelTypes.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs a regen to pass ZAP CI....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be fixed by now! I regenerated the ZAP files and pushed them here
Jenny Gallegos Cárdenas seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
Fixes #38417
Testing
Tested with the same workflows in my own repo, passing most and some only failed due to not pulling docker images properly.