-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
add isLastElement in kotlin tlvReader #28617
base: master
Are you sure you want to change the base?
add isLastElement in kotlin tlvReader #28617
Conversation
PR #28617: Size comparison from a3c4b48 to 29f0f4f Increases (11 builds for bl702, mbed, psoc6, telink)
Decreases (11 builds for bl702l, efr32, nrfconnect, psoc6, telink)
Full report (60 builds for bl602, bl702, bl702l, cc32xx, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
|
Tag.encode(value.toType().encode(), tag).size + value.encode().size | ||
|
||
/** Returns true if this reader is currently pointing to the last element in the TLV data. */ | ||
fun isLastElement(): Boolean { |
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.
can we use =
here since function is trivial?
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 also seems to rely that decoding and re-encoding is the same size. Likely correct, but needs mental processing for this.
How about using the same logic as in peekElement (i.e. remembering the index) and checking if we reached the end index (i.e. index >= bytes.size
)?
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 also seems to throw if you already read the entire tlv ... I would expect isLastElement to return true if the TLV is pointing to the last element and false otherwise. Instead this also has the case of will throw if reader points to the end/is exhausted
which seems inconvenient.
What is the usage of this method? Why don't we have a wasLastElement
maybe which is trivial?
@yunhanw-google could you add some description on why we do this rather than just describing that we do it? It seems trivial to have a |
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
add isLastElement in kotlin tlvReader and corresponding test