-
Notifications
You must be signed in to change notification settings - Fork 4
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
chore: reuse existing functions for calculating protocol fragment #402
Conversation
const size_t metadata_str_size = atclient_atkey_metadata_protocol_strlen(metadata) + 1; | ||
const size_t expected_metadatastr_len = metadata_str_size - 1; | ||
const size_t expected_metadatastr_len = atclient_atkey_metadata_protocol_strlen(metadata); | ||
const size_t metadata_str_size = expected_metadatastr_len + 1; |
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.
flipped these variables so we don't have to +1 then -1
sprintf(*metadata_str + pos, ":ttl:%ld", metadata->ttl); | ||
pos += 5 + atclient_string_utils_long_strlen(metadata->ttl); | ||
sprintf((*metadata_str) + pos, ":ttl:%ld", metadata->ttl); | ||
pos += atclient_atkey_metadata_ttl_strlen(metadata); |
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.
pattern 1: drop in replacement of manual calculation for the method
if (metadata->ccd) { | ||
sprintf(*metadata_str + pos, ":ccd:true"); | ||
pos += 9; | ||
sprintf((*metadata_str) + pos, ":ccd:true"); | ||
} else { | ||
sprintf(*metadata_str + pos, ":ccd:false"); | ||
pos += 10; | ||
sprintf((*metadata_str) + pos, ":ccd:false"); | ||
} | ||
pos += atclient_atkey_metadata_ccd_strlen(metadata); |
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.
pattern 2: we internally call this same if statement inside atclient_atkey_metadata_<thing>_strlen
for all boolean metadata strlen functions, so we can consolidate the pos calculation by moving it outside the if/else
atlogger_log(TAG, ATLOGGER_LOGGING_LEVEL_ERROR, | ||
"metadata_str length mismatch: %lu != %lu\nmetadata:", strlen(*metadata_str), | ||
(expected_metadatastr_len)); | ||
|
||
atlogger_log(TAG, ATLOGGER_LOGGING_LEVEL_ERROR, "metadata_str: %s\n", (*metadata_str)); |
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.
Added some more logging in case this comes up again
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.
LGTM
- What I did
We have methods to calculate the string length of individual metadata fragments, as well as the entire metadata fragment. (fragment as in fragment of an at_protocol verb)
I replaced implementation in the function for calculating entire string length with calls to the individual functions instead of doing each calculation manually.
- How I did it
- How to verify it
See atsign-foundation/noports#1363
- Description for the changelog
chore: reuse existing functions for calculating protocol fragment