Remove MATTER_ATTRIBUTE_FLAG_SINGLETON #39968
Merged
+2,940
−3,006
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
/gemini review |
There was a problem hiding this comment.
Pull Request Overview
This PR removes the unused MATTER_ATTRIBUTE_FLAG_SINGLETON and all associated singleton-handling code and comments to simplify attribute storage and metadata.
- Deleted the singleton attribute flag definition and
IsSingleton()accessor in metadata. - Removed the
singletonAttributeLocationhelper and singleton branch from the read/write logic. - Cleaned up codegen comments to drop references to singleton attributes.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/data-model-providers/codegen/CodegenDataModelProvider.cpp | Removed the bullet about IsSingleton in the NOTE comment block. |
| src/app/util/attribute-storage.cpp | Deleted singletonAttributeLocation and removed singleton branch. |
| src/app/util/attribute-metadata.h | Removed MATTER_ATTRIBUTE_FLAG_SINGLETON definition and method. |
Comments suppressed due to low confidence (2)
src/data-model-providers/codegen/CodegenDataModelProvider.cpp:114
- [nitpick] Align the indentation of the bullet comment with the "NOTE" comment above for consistent formatting. For example, use the same number of leading spaces on each line.
// - IsExternal/IsAutomaticallyPersisted is not used by IM handling
There was a problem hiding this comment.
Code Review
This pull request removes the unused MATTER_ATTRIBUTE_FLAG_SINGLETON flag and its associated code. The changes are mostly correct, but I've pointed out a couple of issues. One is a critical issue where a usage of the removed flag was missed, which will cause a build failure. The other is a medium-severity suggestion to improve code clarity by completely removing a line related to the deleted flag instead of commenting it out.
bzbarsky-apple
approved these changes
Jul 11, 2025
|
PR #39968: Size comparison from 6d5aa8c to a135bf1 Full report (5 builds for cc32xx, stm32, tizen)
|
|
PR #39968: Size comparison from 6d5aa8c to c15f0e0 Full report (11 builds for cc13x4_26x4, cc32xx, qpg, stm32, tizen)
|
tcarmelveilleux
approved these changes
Jul 11, 2025
|
PR #39968: Size comparison from 6d5aa8c to bb7cc6d Full report (57 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, linux, nrfconnect, psoc6, qpg, stm32, telink, tizen)
|
bzbarsky-apple
approved these changes
Jul 11, 2025
…rning that we are this slow though...
|
PR #39968: Size comparison from 6d5aa8c to 81d3da2 Full report (59 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
cecille
approved these changes
Jul 11, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Followup on https://github.com/project-chip/connectedhomeip/pull/39693/files#r2170150515
This flag is never used (grepped for it both here and in zap), so removed it. Maybe we gain a minute flash saving here.
Update:
Testing
This is a delete only. Expecting unit tests to still pass.