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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
