Add support for signed/bool to BufferReader/Writer #27637
Merged
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
Problem: - Safe buffer reader/writer doesn't support signed values - Safe buffer reader/writer doesn't support bools - Fixes project-chip#27629 This PR: - Adds signed and bool support to BufferWriter and to Reader - Adds missing unit tests - Makes it clear that low-level reads are not to be used directly Testing done: - Integration tests pass - New unit tests added and pass - Existing unit tests all pass
pullapprove
bot
requested review from
amitnj,
andy31415,
anush-apple,
arkq,
bzbarsky-apple,
carol-apple,
chrisdecenzo,
chshu,
chulspro,
cliffamzn,
CodeChronos928,
Damian-Nordic,
dhrishi,
electrocucaracha,
emargolis,
gjc13,
harimau-qirex,
harsha-rajendran,
hawk248,
jelderton,
jepenven-silabs,
jmartinez-silabs,
jmeg-sfy,
joonhaengHeo and
jtung-apple
pullapprove
bot
requested review from
kkasperczyk-no,
ksperling-apple,
lazarkov,
lpbeliveau-silabs,
LuDuda,
mrjerryjohns,
msandstedt,
mspang,
nivi-apple,
robszewczyk,
saurabhst,
selissia,
sharadb-amazon,
tecimovic,
tehampson,
turon,
vijs,
vivien-apple,
woody-apple,
xylophone21,
younghak-hwang and
yunhanw-google
|
PR #27637: Size comparison from 7e3aa84 to 4d1f6b0 Increases (58 builds for bl602, bl702, bl702l, cc32xx, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
Decreases (8 builds for bl702, bl702l, cc32xx, psoc6)
Full report (58 builds for bl602, bl702, bl702l, cc32xx, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
|
andy31415
approved these changes
Jul 5, 2023
hicklin
approved these changes
Jul 6, 2023
|
|
||
| namespace chip { | ||
| namespace Encoding { | ||
| namespace LittleEndian { | ||
|
|
||
| namespace { | ||
| // These helper methods return void and put the value being read into an | ||
|
|
||
| // This helper methods return void and put the value being read into an |
There was a problem hiding this comment.
"method returns" and "puts"
| memcpy(&result, p, sizeof(result)); | ||
| result = chip::Encoding::LittleEndian::HostSwap(result); | ||
|
|
||
| *dest = static_cast<T>(result); |
There was a problem hiding this comment.
Casting to signed of things that are "negative" (i.e. out of range) has implementation-defined behavior until c++20...
This should probably use CastToSigned for the casting; that's what we have that utility for.
| { | ||
| static_assert(CHAR_BIT == 8, "Our various sizeof checks rely on bytes and octets being the same thing"); | ||
| static_assert((-1 & 3) == 3, "LittleEndian::BufferReader only works with 2's complement architectures."); |
There was a problem hiding this comment.
Is this the part that's meant to make the static_cast above safe? Or is this something we depend on for the HostSwap bits to work right for negative numbers? Would be good to make it clearer what the actual 2's complement dependency is here.
| { | ||
| while (size > 0) | ||
| { | ||
| Put(static_cast<uint8_t>(x & 0xff)); |
There was a problem hiding this comment.
Would it not be safer to just cast to uint64_t and then EndianPut that? That's obviously the reverse of what we do when reading in the BufferReader, whereas it's not entirely obvious without digging through C++ specs what the bitwise ops here will end up doing.
| BufferWriter(uint8_t * buf, size_t len) : EndianBufferWriterBase<BufferWriter>(buf, len) {} | ||
| BufferWriter(uint8_t * buf, size_t len) : EndianBufferWriterBase<BufferWriter>(buf, len) | ||
| { | ||
| static_assert((-1 & 3) == 3, "LittleEndian::BufferWriter only works with 2's complement architectures."); |
There was a problem hiding this comment.
Again, would be good to document exactly where this dependency comes in. I think if you do cast to unsigned as suggested above there is no such dependency here (because cast to unsigned is well-defined in the C++ standard).
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.
Problem
Changes in this PR
Testing done