[PATCH] D75057: Syndicate, test and fix base64 implementation
Haojian Wu via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Fri Feb 28 03:53:01 PST 2020
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
More information about the cfe-commits
mailing list