Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2493723003:
[inspector] Fix and refactor String16 (Closed)
|
Created:
4 years, 1 month ago by Clemens Hammacher Modified:
4 years, 1 month ago CC:
devtools-reviews_chromium.org, v8-reviews_googlegroups.com Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[inspector] Fix and refactor String16
This CL defines move semantics for String16, and fixes issues with the
hash code not being set correctly on swap or copy.
It also extends the interface by a few handy templates.
All this functionality will be used for the wasm translations, where
String16s are often concatenated and used as keys in hash tables.
BUG=chromium:659715
R=yangguo@chromium.org, kozyatinskiy@chromium.org
Committed: https://crrev.com/c9c6c1a393b2f662e130169ef21864b5a89e8cb3
Cr-Commit-Position: refs/heads/master@{#41007}
Patch Set 1 #
Total comments: 11
Patch Set 2 : Address Dmitry's comments #Patch Set 3 : Define default copy and move constructors and assign operators #Patch Set 4 : Windows unhappy; revert last patch and define move assignment manually #
Total comments: 2
Depends on Patchset: Messages
Total messages: 32 (19 generated)
The CQ bit was checked by clemensh@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 unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
dgozman@chromium.org changed reviewers: + dgozman@chromium.org
Nice stuff! https://codereview.chromium.org/2493723003/diff/1/src/inspector/string-16.h File src/inspector/string-16.h (left): https://codereview.chromium.org/2493723003/diff/1/src/inspector/string-16.h#o... src/inspector/string-16.h:92: inline String16 operator+(const String16& a, const char* b) { Where did this one go? https://codereview.chromium.org/2493723003/diff/1/src/inspector/string-16.h File src/inspector/string-16.h (right): https://codereview.chromium.org/2493723003/diff/1/src/inspector/string-16.h#n... src/inspector/string-16.h:39: String16& operator=(const String16& other) = default; Let's have operator=(const String16&&) as well? https://codereview.chromium.org/2493723003/diff/1/src/inspector/string-16.h#n... src/inspector/string-16.h:79: if (!hash_code) ++hash_code; What if hash_code == -1? https://codereview.chromium.org/2493723003/diff/1/src/inspector/string-16.h#n... src/inspector/string-16.h:118: void append(int); I think these are error-prone, since the semantic is really different from append(UChar), but the signature is similar. Let's call it appendNumber(...).
https://codereview.chromium.org/2493723003/diff/1/src/inspector/string-16.h File src/inspector/string-16.h (left): https://codereview.chromium.org/2493723003/diff/1/src/inspector/string-16.h#o... src/inspector/string-16.h:92: inline String16 operator+(const String16& a, const char* b) { On 2016/11/11 21:41:21, dgozman wrote: > Where did this one go? It's redundant, since there is a String16(const char*) constructor. So String16 + const char* is the same String16 + String16(const char*) anyway. https://codereview.chromium.org/2493723003/diff/1/src/inspector/string-16.h File src/inspector/string-16.h (right): https://codereview.chromium.org/2493723003/diff/1/src/inspector/string-16.h#n... src/inspector/string-16.h:39: String16& operator=(const String16& other) = default; On 2016/11/11 21:41:21, dgozman wrote: > Let's have operator=(const String16&&) as well? Sure, I will add it when rebasing. https://codereview.chromium.org/2493723003/diff/1/src/inspector/string-16.h#n... src/inspector/string-16.h:79: if (!hash_code) ++hash_code; On 2016/11/11 21:41:21, dgozman wrote: > What if hash_code == -1? Then !hash_code will be false, so it stays -1. https://codereview.chromium.org/2493723003/diff/1/src/inspector/string-16.h#n... src/inspector/string-16.h:118: void append(int); On 2016/11/11 21:41:21, dgozman wrote: > I think these are error-prone, since the semantic is really different from > append(UChar), but the signature is similar. Let's call it appendNumber(...). I see the problem. Renaming the functions would make the appendAll magic more difficult though, requiring template specialization for all types where the append function has a different name. Is this worth it? For the appendAll function, the user has to be careful anyway not to confuse int or size_t with UChar.
https://codereview.chromium.org/2493723003/diff/1/src/inspector/string-16.h File src/inspector/string-16.h (right): https://codereview.chromium.org/2493723003/diff/1/src/inspector/string-16.h#n... src/inspector/string-16.h:79: if (!hash_code) ++hash_code; On 2016/11/14 06:02:07, Clemens Hammacher wrote: > On 2016/11/11 21:41:21, dgozman wrote: > > What if hash_code == -1? > > Then !hash_code will be false, so it stays -1. Ops, my bad :-) https://codereview.chromium.org/2493723003/diff/1/src/inspector/string-16.h#n... src/inspector/string-16.h:118: void append(int); On 2016/11/14 06:02:08, Clemens Hammacher wrote: > On 2016/11/11 21:41:21, dgozman wrote: > > I think these are error-prone, since the semantic is really different from > > append(UChar), but the signature is similar. Let's call it appendNumber(...). > > I see the problem. Renaming the functions would make the appendAll magic more > difficult though, requiring template specialization for all types where the > append function has a different name. Is this worth it? For the appendAll > function, the user has to be careful anyway not to confuse int or size_t with > UChar. Maybe don't support integer than? Why not pass String::fromInteger(x) instead, even to appendAll?
https://codereview.chromium.org/2493723003/diff/1/src/inspector/string-16.h File src/inspector/string-16.h (right): https://codereview.chromium.org/2493723003/diff/1/src/inspector/string-16.h#n... src/inspector/string-16.h:118: void append(int); On 2016/11/14 20:32:52, dgozman wrote: > On 2016/11/14 06:02:08, Clemens Hammacher wrote: > > On 2016/11/11 21:41:21, dgozman wrote: > > > I think these are error-prone, since the semantic is really different from > > > append(UChar), but the signature is similar. Let's call it > appendNumber(...). > > > > I see the problem. Renaming the functions would make the appendAll magic more > > difficult though, requiring template specialization for all types where the > > append function has a different name. Is this worth it? For the appendAll > > function, the user has to be careful anyway not to confuse int or size_t with > > UChar. > > Maybe don't support integer than? Why not pass String::fromInteger(x) instead, > even to appendAll? Hm, that degrades performance and readability, which also propagates to the String16::concat method. See v8-debugger-agent-impl.cc for an example that would be much more verbose with explicit String16::fromInteger calls. Do you insist on this change? ;)
On 2016/11/14 21:11:02, Clemens Hammacher wrote: > https://codereview.chromium.org/2493723003/diff/1/src/inspector/string-16.h > File src/inspector/string-16.h (right): > > https://codereview.chromium.org/2493723003/diff/1/src/inspector/string-16.h#n... > src/inspector/string-16.h:118: void append(int); > On 2016/11/14 20:32:52, dgozman wrote: > > On 2016/11/14 06:02:08, Clemens Hammacher wrote: > > > On 2016/11/11 21:41:21, dgozman wrote: > > > > I think these are error-prone, since the semantic is really different from > > > > append(UChar), but the signature is similar. Let's call it > > appendNumber(...). > > > > > > I see the problem. Renaming the functions would make the appendAll magic > more > > > difficult though, requiring template specialization for all types where the > > > append function has a different name. Is this worth it? For the appendAll > > > function, the user has to be careful anyway not to confuse int or size_t > with > > > UChar. > > > > Maybe don't support integer than? Why not pass String::fromInteger(x) instead, > > even to appendAll? > > Hm, that degrades performance and readability, which also propagates to the > String16::concat method. See v8-debugger-agent-impl.cc for an example that would > be much more verbose with explicit String16::fromInteger calls. > Do you insist on this change? ;) Performance difference is not a big deal here, but safety is important. I'd argue that readability is actually degraded - I have to carefully check integer types. What it takes to instead remove append(char/UChar)? Could that be easier and more convenient?
The CQ bit was checked by clemensh@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 clemensh@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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_win64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng/builds/17789)
The CQ bit was checked by clemensh@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...
On 2016/11/14 at 22:38:19, dgozman wrote: > On 2016/11/14 21:11:02, Clemens Hammacher wrote: > > https://codereview.chromium.org/2493723003/diff/1/src/inspector/string-16.h > > File src/inspector/string-16.h (right): > > > > https://codereview.chromium.org/2493723003/diff/1/src/inspector/string-16.h#n... > > src/inspector/string-16.h:118: void append(int); > > On 2016/11/14 20:32:52, dgozman wrote: > > > On 2016/11/14 06:02:08, Clemens Hammacher wrote: > > > > On 2016/11/11 21:41:21, dgozman wrote: > > > > > I think these are error-prone, since the semantic is really different from > > > > > append(UChar), but the signature is similar. Let's call it > > > appendNumber(...). > > > > > > > > I see the problem. Renaming the functions would make the appendAll magic > > more > > > > difficult though, requiring template specialization for all types where the > > > > append function has a different name. Is this worth it? For the appendAll > > > > function, the user has to be careful anyway not to confuse int or size_t > > with > > > > UChar. > > > > > > Maybe don't support integer than? Why not pass String::fromInteger(x) instead, > > > even to appendAll? > > > > Hm, that degrades performance and readability, which also propagates to the > > String16::concat method. See v8-debugger-agent-impl.cc for an example that would > > be much more verbose with explicit String16::fromInteger calls. > > Do you insist on this change? ;) > > Performance difference is not a big deal here, but safety is important. I'd argue that readability is actually degraded - I have to carefully check integer types. > What it takes to instead remove append(char/UChar)? Could that be easier and more convenient? I would create just the same issues. I now renamed the methods to appendNumber; users will have to either manually append them now without using appendAll or String16::concat, or wrap them in String16::fromInteger. PTAL.
lgtm https://codereview.chromium.org/2493723003/diff/60001/src/inspector/v8-debugg... File src/inspector/v8-debugger-agent-impl.cc (right): https://codereview.chromium.org/2493723003/diff/60001/src/inspector/v8-debugg... src/inspector/v8-debugger-agent-impl.cc:79: String16Builder builder; Just curious why builder and not concat with fromInteger()?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2493723003/diff/60001/src/inspector/v8-debugg... File src/inspector/v8-debugger-agent-impl.cc (right): https://codereview.chromium.org/2493723003/diff/60001/src/inspector/v8-debugg... src/inspector/v8-debugger-agent-impl.cc:79: String16Builder builder; On 2016/11/15 at 15:32:25, dgozman wrote: > Just curious why builder and not concat with fromInteger()? Performance ;)
The CQ bit was checked by clemensh@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
This CL has an open dependency (Issue 2493823003 Patch 60001). Please resolve the dependency and try again. If you are sure that there is no real dependency, please use one of the options listed in https://goo.gl/9Es4OR to land the CL.
The CQ bit was checked by clemensh@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== [inspector] Fix and refactor String16 This CL defines move semantics for String16, and fixes issues with the hash code not being set correctly on swap or copy. It also extends the interface by a few handy templates. All this functionality will be used for the wasm translations, where String16s are often concatenated and used as keys in hash tables. BUG=chromium:659715 R=yangguo@chromium.org, kozyatinskiy@chromium.org ========== to ========== [inspector] Fix and refactor String16 This CL defines move semantics for String16, and fixes issues with the hash code not being set correctly on swap or copy. It also extends the interface by a few handy templates. All this functionality will be used for the wasm translations, where String16s are often concatenated and used as keys in hash tables. BUG=chromium:659715 R=yangguo@chromium.org, kozyatinskiy@chromium.org Committed: https://crrev.com/c9c6c1a393b2f662e130169ef21864b5a89e8cb3 Cr-Commit-Position: refs/heads/master@{#41007} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/c9c6c1a393b2f662e130169ef21864b5a89e8cb3 Cr-Commit-Position: refs/heads/master@{#41007} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
