Update metadata-tree from MetadaList Foo(path) to CHIP_ERROR Foo(path, ListBuilder &)
#37127
Conversation
This file contains 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
|
Review changes with |
andy31415
force-pushed
the
append_only_api
branch
6 times, most recently
from
bd9492b to
a39551e
Compare
andy31415
force-pushed
the
append_only_api
branch
from
a39551e to
92e448d
Compare
|
PR #37127: Size comparison from 2ecbcc2 to c19a4fd Full report (14 builds for cc13x4_26x4, cc32xx, nrfconnect, qpg, stm32, tizen)
|
|
PR #37127: Size comparison from 2ecbcc2 to 48dc77c Full report (14 builds for cc13x4_26x4, cc32xx, nrfconnect, qpg, stm32, tizen)
|
src/app/clusters/microwave-oven-control-server/microwave-oven-control-server.cpp
Outdated
Show resolved
Hide resolved
src/app/clusters/microwave-oven-control-server/microwave-oven-control-server.cpp
Outdated
Show resolved
Hide resolved
Co-authored-by: Tennessee Carmel-Veilleux <tennessee.carmelveilleux@gmail.com>
Co-authored-by: Tennessee Carmel-Veilleux <tennessee.carmelveilleux@gmail.com>
Co-authored-by: Tennessee Carmel-Veilleux <tennessee.carmelveilleux@gmail.com>
Co-authored-by: Tennessee Carmel-Veilleux <tennessee.carmelveilleux@gmail.com>
Co-authored-by: Tennessee Carmel-Veilleux <tennessee.carmelveilleux@gmail.com>
Co-authored-by: Tennessee Carmel-Veilleux <tennessee.carmelveilleux@gmail.com>
tcarmelveilleux
approved these changes
Jan 20, 2025
…control-server.cpp Co-authored-by: Tennessee Carmel-Veilleux <tennessee.carmelveilleux@gmail.com>
…p into append_only_api
|
PR #37127: Size comparison from 2ecbcc2 to 6ae480e Full report (71 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
|
PR #37127: Size comparison from b5cdd21 to eaf9f0c Full report (71 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
This was referenced Jan 21, 2025
jmartinez-silabs
approved these changes
Jan 23, 2025
Co-authored-by: Junior Martinez <67972863+jmartinez-silabs@users.noreply.github.com>
|
PR #37127: Size comparison from 575a440 to ded4d53 Full report (71 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
| if (mBuffer == nullptr) | ||
| { | ||
| mBuffer = static_cast<uint8_t *>(Platform::MemoryCalloc(numElements, mElementSize)); | ||
| mCapacity = numElements; |
There was a problem hiding this comment.
Do we really want to set a nonzero mCapacity if allocation failed?
| mBuffer = const_cast<uint8_t *>(static_cast<const uint8_t *>(buffer)); | ||
| mElementCount = numElements; | ||
| // The assertions below are because we know the buffer is null/not allocated yet | ||
| VerifyOrDie(mCapacity == 0); |
There was a problem hiding this comment.
Yeah, except above we set mCapacity to be nonzero on allocation failure, right? And we set mBufferIsAllocated to true too...
There was a problem hiding this comment.
That was an oversight. Moved the return on alloc failure.
| /// Ensure that at least the specified number of elements | ||
| /// can be appended to the internal buffer; | ||
| /// | ||
| /// This will cause the internal buffer to become and allocated buffer |
| if ((mBuffer != nullptr) && mAllocated) | ||
| ReturnErrorOnFailure(EnsureAppendCapacity(numElements)); | ||
|
|
||
| memcpy(mBuffer + mElementCount * mElementSize, buffer, numElements * mElementSize); |
There was a problem hiding this comment.
Should be memmove, because nothing guarantees that buffer does not point somewhere inside mBuffer here.
There was a problem hiding this comment.
That would be bad, because EnsureAppendCapacity may reallocate/move data, so if the buffer we receive points inside itself, it would not be moved.
In theory possible (e.g. by calling EnsureAppendCapacity first) however it seems like a pattern we should not support. I would rather place this in documentation/requirements because of that. It is a footgun to try to append from itself. Also some logical error because we mostly want unique things in our array.
There was a problem hiding this comment.
Probaly in the current design not even possible because the builders do not provide external access to the buffer. Adding docs and __restrict__
| void SetImmutable() const { mIsImmutable = true; } | ||
| /// Represents a RAII instance owning a buffer. | ||
| /// | ||
| /// It auto-frees the owned buffer on destruction |
There was a problem hiding this comment.
Using what deallocator? Should be documented.
| @@ -186,6 +186,12 @@ class ProviderMetadataTree | |||
| /// TODO: We should remove this function when the AttributeAccessInterface/CommandHandlerInterface is able to report | |||
| /// the attribute changes. | |||
| virtual void Temporary_ReportAttributeChanged(const AttributePathParams & path) = 0; | |||
|
|
|||
| // "convenience" functions that just return the data and ignore the error | |||
| // This returns the builder as-is even after the error (e.g. not found would return empty data) | |||
There was a problem hiding this comment.
This is not in fact returning the builder.
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.
Made metadata list methods to return CHIP_ERROR and have out arguments. This has the benefit of:
CHIP_ERRORis propagated and we make it clear where we do not actually expect errors (via methods namedIgnoreErrorprovider1->Clusters(endpointID, list); provider2->Cluster(endpointID, list)Changes are expected to be roughly flash-size neutral. Some flash increases due to descriptor.cpp now handling errors should be offset by some common code reuse.
Testing
Unit tests updated and added.
This is common code with no functionality difference, so significant coverage exists.