Part of ENG-3408.This one implements base64 encode, which replaces the slow JS implementation.
This implementation is inspired by this article but it is somehow modified (original article impl requires C++20).
Depends on D7485
Details
Details
Called this from JS for various cases and compared it with online converter. Results are equal for each padding value (0, 1, 2).
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| native/cpp/CommonCpp/NativeModules/CommUtilsModule.cpp | ||
|---|---|---|
| 102 ↗ | (On Diff #25269) | just wondering, is there a reason for this to be synchronous instead of promise? |
| native/cpp/CommonCpp/Tools/Base64.cpp | ||
| 6 ↗ | (On Diff #25269) | I prefer the original name from article, I think alphabet is confusing as this name stands for standardized set of letter, not special characters like '+', '/' |
| 15–18 ↗ | (On Diff #25269) | can you make 63 a constant with some meaningful name to improve readability? SIX_BIT_MASK or something |
| native/cpp/CommonCpp/Tools/Base64.h | ||
| 1 ↗ | (On Diff #25269) | you should add #pragma once, docs |
| 4 ↗ | (On Diff #25269) | I think creating a class with a static method will be more readable and will match the pattern from the rest of the cpp codebase. Don't you think? |
| native/cpp/CommonCpp/NativeModules/CommUtilsModule.cpp | ||
|---|---|---|
| 102 ↗ | (On Diff #25269) | Yes, this is to keep compatibility with our JS impl as well as other solutions like base64-js. I looked into many RN/JS base64 implementations and all of them are synchronous, the same way as JSON.stringify() is sync. |
| native/cpp/CommonCpp/Tools/Base64.cpp | ||
| 6 ↗ | (On Diff #25269) | As you wish |
| 15–18 ↗ | (On Diff #25269) | Sure |
| native/cpp/CommonCpp/Tools/Base64.h | ||
| 4 ↗ | (On Diff #25269) | I'd prefer not to introduce classes if unnecessary. But I'm not opposed to namespaces |
| native/cpp/CommonCpp/Tools/Base64.h | ||
|---|---|---|
| 4 ↗ | (On Diff #25269) | Never mind, I see this pattern is indeed everywhere else in our codebase. Not very happy about it but will follow it; I don't want to introduce inconsistencies |
| native/cpp/CommonCpp/NativeModules/CommUtilsModule.cpp | ||
|---|---|---|
| 102 ↗ | (On Diff #25269) | right, thanks for explanation! |