[services] Backup - Add function to get utf8 string length
AbandonedPublic
Actions

Authored by • jakub on Jul 12 2022, 1:19 AM.

Details

Reviewers

tomek
• karol
ashoat

Summary

Depends on D4504
context: here

Chars encoded in utf8 have different size between 1 and 4 bytes. Standard length method return different values on different architectures. To avoid that, we need to have an universal function for counting string bytes.

Test Plan

cd services && yarn run-unit-tests backup

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

• jakub created this revision.Jul 12 2022, 1:19 AM

Herald added subscribers: • abosh, • karol, atul and 3 others. · View Herald TranscriptJul 12 2022, 1:19 AM

• jakub edited the summary of this revision. (Show Details)Jul 12 2022, 1:23 AM

• jakub added a parent revision: D4504: [services] Backup - Remove unnecessary brackets in unit test.

Harbormaster completed remote builds in B10462: Diff 14407.Jul 12 2022, 1:35 AM

• jakub requested review of this revision.Jul 12 2022, 1:35 AM

• jakub edited the summary of this revision. (Show Details)Jul 12 2022, 2:20 AM

• jakub edited the test plan for this revision. (Show Details)

• jakub edited the summary of this revision. (Show Details)Jul 12 2022, 2:22 AM

Surprising that there is no direct library function for this! Did some quick Googling and found this StackOverflow, but couldn't find any single API that would return the size of a UTF-8 string.

Additionally – reminder that you should ALWAYS specify a reviewer for a diff!

• jakub added reviewers: tomek, • karol.Jul 13 2022, 5:10 AM

• jakub added a child revision: D4544: [services] Backup - Add function to get utf8 string length.Jul 15 2022, 3:02 AM

Looks ok, personally, I'd try to do this without pointers but it probably doesn't matter.

Can this function be called from multiple threads?

services/backup/src/Tools.cpp
60	Before this while in https://en.cppreference.com/w/cpp/string/multibyte/mblen there's a line `std::mblen(nullptr, 0); // reset the conversion state`. Should we include it?
63	What is `strlen_mb()` in this error message? Could you modify the message so that it is more helpful?

This revision now requires changes to proceed.Jul 18 2022, 5:46 AM

It seems to me that this diff should be abandoned since it's replaced in D4544. It's important that you think in terms of diffs going forward... when you fix an issue introduced in an earlier diff, you should always update the existing diff instead of introducing a new one.

Think about it from the perspective of the reviewer. What is the point of reviewing this code if it's going to immediately be thrown away anyways?

• jakub abandoned this revision.Jul 26 2022, 8:27 AM

ashoat mentioned this in D4544: [services] Backup - Add function to get utf8 string length.Jul 26 2022, 12:06 PM