Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
706063002:
for X in {Dec,BigDec,Scalar}, SkWStream::write"X"AsText no longer mallocs (Closed)
|
Created:
6 years, 1 month ago by hal.canary Modified:
6 years, 1 month ago CC:
reviews_skia.org Base URL:
https://skia.googlesource.com/skia.git@master Project:
skia Visibility:
Public. |
Descriptionfor X in {Dec,BigDec,Scalar}, SkWStream::write"X"AsText no longer mallocs
Committed: https://skia.googlesource.com/skia/+/f0de423f0930c7ab2b4d722fd23ce68533363443
Patch Set 1 #
Total comments: 3
Messages
Total messages: 14 (3 generated)
halcanary@google.com changed reviewers: + bungeman@google.com, reed@google.com
ptal priority=minimal
mtklein@google.com changed reviewers: + mtklein@google.com
lgtm
https://codereview.chromium.org/706063002/diff/1/src/core/SkStream.cpp File src/core/SkStream.cpp (right): https://codereview.chromium.org/706063002/diff/1/src/core/SkStream.cpp#newcode90 src/core/SkStream.cpp:90: char buffer[SkStrAppendS32_MaxSize]; does the buffer need to be this const + 1, to have a slot for the terminating 0?
On 2014/11/06 21:45:10, reed1 wrote: > https://codereview.chromium.org/706063002/diff/1/src/core/SkStream.cpp > File src/core/SkStream.cpp (right): > > https://codereview.chromium.org/706063002/diff/1/src/core/SkStream.cpp#newcode90 > src/core/SkStream.cpp:90: char buffer[SkStrAppendS32_MaxSize]; > does the buffer need to be this const + 1, to have a slot for the terminating 0? other than that question, lgtm
https://codereview.chromium.org/706063002/diff/1/src/core/SkStream.cpp File src/core/SkStream.cpp (right): https://codereview.chromium.org/706063002/diff/1/src/core/SkStream.cpp#newcode90 src/core/SkStream.cpp:90: char buffer[SkStrAppendS32_MaxSize]; On 2014/11/06 21:45:09, reed1 wrote: > does the buffer need to be this const + 1, to have a slot for the terminating 0? SkWStream::write(void*, size_t) doesn't need a terminating '\0' and those functions do not insert one. See SkString::insertS32 for an example.
The CQ bit was checked by halcanary@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/706063002/1
Message was sent while issue was closed.
Committed patchset #1 (id:1) as f0de423f0930c7ab2b4d722fd23ce68533363443
Message was sent while issue was closed.
https://codereview.chromium.org/706063002/diff/1/src/core/SkStream.cpp File src/core/SkStream.cpp (right): https://codereview.chromium.org/706063002/diff/1/src/core/SkStream.cpp#newcode90 src/core/SkStream.cpp:90: char buffer[SkStrAppendS32_MaxSize]; On 2014/11/06 22:26:30, Hal Canary wrote: > On 2014/11/06 21:45:09, reed1 wrote: > > does the buffer need to be this const + 1, to have a slot for the terminating > 0? > > SkWStream::write(void*, size_t) doesn't need a terminating '\0' and those > functions do not insert one. See SkString::insertS32 for an example. Ah, right. I'll add some dox so I can remind myself in SkString.h
Message was sent while issue was closed.
On 2014/11/07 14:52:28, reed1 wrote: > https://codereview.chromium.org/706063002/diff/1/src/core/SkStream.cpp > File src/core/SkStream.cpp (right): > > https://codereview.chromium.org/706063002/diff/1/src/core/SkStream.cpp#newcode90 > src/core/SkStream.cpp:90: char buffer[SkStrAppendS32_MaxSize]; > On 2014/11/06 22:26:30, Hal Canary wrote: > > On 2014/11/06 21:45:09, reed1 wrote: > > > does the buffer need to be this const + 1, to have a slot for the > terminating > > 0? > > > > SkWStream::write(void*, size_t) doesn't need a terminating '\0' and those > > functions do not insert one. See SkString::insertS32 for an example. > > Ah, right. > > I'll add some dox so I can remind myself in SkString.h while you're at it, add a TODO to make a hexadecimal version of SkStrAppendU64.
Message was sent while issue was closed.
On 2014/11/07 15:25:17, Hal Canary wrote: > On 2014/11/07 14:52:28, reed1 wrote: > > https://codereview.chromium.org/706063002/diff/1/src/core/SkStream.cpp > > File src/core/SkStream.cpp (right): > > > > > https://codereview.chromium.org/706063002/diff/1/src/core/SkStream.cpp#newcode90 > > src/core/SkStream.cpp:90: char buffer[SkStrAppendS32_MaxSize]; > > On 2014/11/06 22:26:30, Hal Canary wrote: > > > On 2014/11/06 21:45:09, reed1 wrote: > > > > does the buffer need to be this const + 1, to have a slot for the > > terminating > > > 0? > > > > > > SkWStream::write(void*, size_t) doesn't need a terminating '\0' and those > > > functions do not insert one. See SkString::insertS32 for an example. > > > > Ah, right. > > > > I'll add some dox so I can remind myself in SkString.h > > while you're at it, add a TODO to make a hexadecimal version of SkStrAppendU64. Hmm. zero-filled? upper-case/lower-case? I suggest we file a bug to discuss it first.
Message was sent while issue was closed.
On 2014/11/07 15:31:09, reed1 wrote:
> Hmm. zero-filled? upper-case/lower-case? I suggest we file a bug to discuss it
> first.
do exactly what
void SkString::appendHex(uint32_t value, int minDigits = 0);
does.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
