Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2956603002:
Add StringBuilder::erase(index) (Closed)
DescriptionAdd StringBuilder::erase(index)
The function name follows the WTF Vector convention.
Needed in LayoutNG.
BUG=636993
Review-Url: https://codereview.chromium.org/2956603002
Cr-Commit-Position: refs/heads/master@{#481917}
Committed: https://chromium.googlesource.com/chromium/src/+/37fd3477f0a335abbc3f5ebf6b8121e0be19edba
Patch Set 1 : StringBuilder.erase #
Total comments: 7
Patch Set 2 : Removed last character optimization #
Messages
Total messages: 20 (13 generated)
The CQ bit was checked by kojii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by kojii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:1) has been deleted
Description was changed from ========== StringBuilder.erase BUG= ========== to ========== Add StringBuilder::erase(index) The function name follows the WTF Vector convention. Needed in LayoutNG. BUG=636993 ==========
kojii@chromium.org changed reviewers: + haraken@chromium.org
PTAL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
https://codereview.chromium.org/2956603002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/wtf/text/StringBuilder.cpp (right): https://codereview.chromium.org/2956603002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/wtf/text/StringBuilder.cpp:240: EnsureBuffer8(0); Do you need this? https://codereview.chromium.org/2956603002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/wtf/text/StringBuilder.cpp:243: EnsureBuffer16(0); Ditto. https://codereview.chromium.org/2956603002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/wtf/text/StringBuilder.cpp:246: --length_; Do you want to call Resize() in this case as well? It looks inconsistent that you call Resize() only when index == length_ - 1.
Thank you for the prompt review! Replies inline: https://codereview.chromium.org/2956603002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/wtf/text/StringBuilder.cpp (right): https://codereview.chromium.org/2956603002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/wtf/text/StringBuilder.cpp:240: EnsureBuffer8(0); On 2017/06/23 at 13:16:49, haraken wrote: > Do you need this? Yes, since StringBuilder may have StringImpl but not buffer yet. This ensures there's a buffer, and StringImpl is copied into it. I can call it only when "!HasBuffer()", but that's all what "EnsureBuffer" does; it calls CreateBuffer() if !HasBuffer. https://codereview.chromium.org/2956603002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/wtf/text/StringBuilder.cpp:246: --length_; On 2017/06/23 at 13:16:49, haraken wrote: > Do you want to call Resize() in this case as well? > > It looks inconsistent that you call Resize() only when index == length_ - 1. I guess you misunderstood something, probably due to micro-optimization I put in? Resize() does two things: 1. Change buffer8_/buffer16_. 2. Change length_. Since this function changes buffer8_/buffer16_, we can't call Resize() here. The Resize() call, as mentioned above, is a suspective micro-optimization for when removing the last character, I guess I shouldn't put it in if it makes readability worse. Should I?
LGTM https://codereview.chromium.org/2956603002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/wtf/text/StringBuilder.cpp (right): https://codereview.chromium.org/2956603002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/wtf/text/StringBuilder.cpp:246: --length_; On 2017/06/23 13:45:23, kojii wrote: > On 2017/06/23 at 13:16:49, haraken wrote: > > Do you want to call Resize() in this case as well? > > > > It looks inconsistent that you call Resize() only when index == length_ - 1. > > I guess you misunderstood something, probably due to micro-optimization I put > in? Resize() does two things: > 1. Change buffer8_/buffer16_. > 2. Change length_. > Since this function changes buffer8_/buffer16_, we can't call Resize() here. > > The Resize() call, as mentioned above, is a suspective micro-optimization for > when removing the last character, I guess I shouldn't put it in if it makes > readability worse. Should I? Makes sense. Yeah, remove it or add a comment.
The CQ bit was checked by kojii@chromium.org
https://codereview.chromium.org/2956603002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/wtf/text/StringBuilder.cpp (right): https://codereview.chromium.org/2956603002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/wtf/text/StringBuilder.cpp:246: --length_; On 2017/06/23 at 13:56:17, haraken wrote: > Makes sense. Yeah, remove it or add a comment. Thank you for prompt reply, removed.
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org Link to the patchset: https://codereview.chromium.org/2956603002/#ps40001 (title: "Removed last character optimization")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1498226659171670,
"parent_rev": "47c94aa794bdf214c040ad45de59384a745186fa", "commit_rev":
"37fd3477f0a335abbc3f5ebf6b8121e0be19edba"}
Message was sent while issue was closed.
Description was changed from ========== Add StringBuilder::erase(index) The function name follows the WTF Vector convention. Needed in LayoutNG. BUG=636993 ========== to ========== Add StringBuilder::erase(index) The function name follows the WTF Vector convention. Needed in LayoutNG. BUG=636993 Review-Url: https://codereview.chromium.org/2956603002 Cr-Commit-Position: refs/heads/master@{#481917} Committed: https://chromium.googlesource.com/chromium/src/+/37fd3477f0a335abbc3f5ebf6b81... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:40001) as https://chromium.googlesource.com/chromium/src/+/37fd3477f0a335abbc3f5ebf6b81... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
