[PATCH] D126254: Add support for decoding base64.

[serge via Phabricator via llvm-commits]

serge-sans-paille added inline comments.


================
Comment at: llvm/include/llvm/Support/Base64.h:58
+llvm::Error decodeBase64(llvm::StringRef Bytes, OutputBytes &Output) {
+  constexpr char DecodeTable[] = {
+      64, 64, 64, 64, 64, 64, 64, 64, 64, 64, 64, 64, 64, 64, 64, 64, 64, 64,
----------------
if declared static, this will go in RO data and lead to more efficient code, see https://godbolt.org/z/z4zsqKqEn


================
Comment at: llvm/include/llvm/Support/Base64.h:66
+      37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, 49, 50, 51};
+  auto decodeBase64Byte = [](uint8_t Ch) -> char {
+    if (Ch >= sizeof(DecodeTable))
----------------
Shouldn't `DecodeTable` be part of the capture list?


================
Comment at: llvm/include/llvm/Support/Base64.h:77
+    return createStringError(std::errc::illegal_byte_sequence,
+                             "Base64 encoded strings must be a multiuple of 4 "
+                             "bytes in length");
----------------
nit: multiple


================
Comment at: llvm/include/llvm/Support/Base64.h:85
+      const char Byte = Bytes[Idx + ByteIdx];
+      if (Byte == '=') {
+        if (ByteIdx <= 1) {
----------------
I wonder if the code would be simpler to read if the handling of the trailing '=' would be in a separate loop, that way we would have a first loop that does the fast and straight-forward processing of the stream, then a slightly more complex code to handle the trailing bytes?


================
Comment at: llvm/include/llvm/Support/Base64.h:100
+      }
+      if (Hex64Bytes[ByteIdx] == 64)
+        return createStringError(
----------------
this `if` could be nested in above `else`


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126254/new/

https://reviews.llvm.org/D126254