[PATCH] D75057: Syndicate, test and fix base64 implementation

[Haojian Wu via Phabricator via llvm-commits]

hokein accepted this revision.
hokein added a comment.
This revision is now accepted and ready to land.

In D75057#1896114 <https://reviews.llvm.org/D75057#1896114>, @serge-sans-paille wrote:

> In D75057#1895550 <https://reviews.llvm.org/D75057#1895550>, @hokein wrote:
>
> > thanks for doing this! didn't look into details yet. Could you explain what was the bug in the previous code?
>
>
> Yeah, the code was using + operator instead of | to combine the results, which is a problem when shifting signed values. `(0xFF << 16)` is implicitly converted to a (signed) int, and thus results in `0xffff0000`, which is negative. Combining negative numbers with a `+` in that context is not what we want to do.


fair enough, thanks for the clarification.



================
Comment at: llvm/unittests/Support/Base64Test.cpp:30
+  // from: https://tools.ietf.org/html/rfc4648#section-10
+  TestBase64("", "");
+  TestBase64("f", "Zg==");
----------------
serge-sans-paille wrote:
> hokein wrote:
> > nit: i would just inline the `TestBase64`.
> I wouldn't :-) I find it easier to read like that, if that's okay with you.
personal I don't think it is easier comparing with inline them, `EXPECT_EQ(encodeBase64("f"), "abc");` seems clearer to me, up to you.


================
Comment at: llvm/unittests/Support/Base64Test.cpp:42
+                               0x00, 0x08, (char)0xff, (char)0xee};
+  TestBase64(StringRef(NonPrintableVector, sizeof(NonPrintableVector)),
+             "AAAARgAI/+4=");
----------------
nit:  explicit `StringRef` is not needed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75057