Page MenuHomePhabricator

[native] Add C++ function to decode base64
ClosedPublic

Authored by bartek on May 9 2023, 6:18 AM.
Tags
None
Referenced Files
F1267866: D7758.diff
Sun, Mar 3, 4:23 AM
Unknown Object (File)
Sat, Feb 10, 11:24 AM
Unknown Object (File)
Sat, Feb 10, 11:23 AM
Unknown Object (File)
Sat, Feb 10, 11:22 AM
Unknown Object (File)
Sat, Feb 10, 10:38 AM
Unknown Object (File)
Sat, Feb 10, 10:27 AM
Unknown Object (File)
Fri, Feb 9, 1:42 PM
Unknown Object (File)
Wed, Feb 7, 2:38 AM
Subscribers

Details

Summary

The thumbhash bytes will be stored as base64 string so we need a function to decode it.

This code, similarly to the encode function, is based on this article, with slight modifications.

Test Plan

This will be tested together with the next diff.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

bartek held this revision as a draft.
bartek published this revision for review.May 9 2023, 7:26 AM

Are we doing this so performance is better than it would be in a pure-JS implementation? Given you've already done it I guess we should keep it (assuming you avoided copy-paste), but in the future when we need performant low-level implementations of crypto functionality, I would if it would be easier to bridge to a Rust library than to implement "from scratch" in C++

Looks good, I am a bit afraid there are too many similarities between this and source code from the article, but you said it's based on the article so assuming it was only an inspiration.

native/cpp/CommonCpp/Tools/Base64.cpp
69 ↗(On Diff #26296)

I think to use this you should add #include <algorithm> and #include <iterator>

75 ↗(On Diff #26296)

in line 69 you used end(encoded_str) - 2, so to make it consistent and more readable I will prefer something like:
const auto last = end(encoded_str) - 1; and base logic in next lines on this

120 ↗(On Diff #26296)

I think there was some discussion that initialization with curly brackets`{}` is better, but I can not find it right now

This revision is now accepted and ready to land.May 15 2023, 2:13 AM

If this was copy-pasted you should follow the third_party approach we've outlined before and include the LICENSE

native/cpp/CommonCpp/Tools/Base64.cpp
120 ↗(On Diff #26296)

Here's the snippet I read that encouraged braced initialization as a "best practice":

ab0309.png (1×842 px, 272 KB)

In practice we use all sorts of initialization in our C++ code, but we did say in our C++ coding standards that we "prefer braced initialization": https://www.notion.so/commapp/C-coding-standards-8c9f22b4b16a41c3a868eb5f537db1de?pvs=4#7de58b7ac3c0402bbae2a4e3b6b6738c

This revision was automatically updated to reflect the committed changes.