hubert.reinterpretcast added inline comments.
================
Comment at: llvm/lib/Support/FormattedStream.cpp:59
+ size_t NumBytes = getNumBytesForUTF8(PartialUTF8Char[0]);
+ if (NumBytes > (PartialUTF8Char.size() + Size)) {
+ // If we still don't have enough bytes for a complete code point, just
----------------
The overflow-free version should be preferred: `Size < NumBytes -
PartialUTF8Char.size()`
Considering that `NumBytes - PartialUTF8Char.size()` is already used in the
`else`, this might as well be named `NumBytesNeededFromBuffer` first (at which
point it would be the only use of `NumBytes`, so we can get rid of `NumBytes`):
```
size_t NumBytesNeededFromBuffer =
getNumBytesForUTF8(PartialUTF8Char[0]) - PartialUTF8Char.size();
if (Size < NumBytesNeededFromBuffer) {
```
================
Comment at: llvm/lib/Support/FormattedStream.cpp:87
+ // even if the buffer is being flushed.
+ if ((Ptr + NumBytes) > End) {
+ PartialUTF8Char = StringRef(Ptr, End - Ptr);
----------------
Prefer the overflow-free version: `End - Ptr < NumBytes`. If inclined to name
`End - Ptr` (it would occur twice), `BytesAvailable` makes sense.
================
Comment at: llvm/unittests/Support/formatted_raw_ostream_test.cpp:88
+
+TEST(formatted_raw_ostreamTest, Test_UTF8) {
+ SmallString<128> A;
----------------
ostannard wrote:
> hubert.reinterpretcast wrote:
> > Should there be a test for combining characters?
> This doesn't support combining characters, I don't think there's much point
> in adding a test for something which doesn't work.
Got it; thanks.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D76291/new/
https://reviews.llvm.org/D76291
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
