Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(284)

Issue 5665229014827008: Issue 150 - [Typed objects] Implement string type (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
5 years, 6 months ago by Wladimir Palant
Modified:
5 years, 4 months ago
CC:
Felix Dahlke
Visibility:
Public.

Description

Issue 150 - [Typed objects] Implement string type

Patch Set 1 #

Total comments: 8

Patch Set 2 : #

Unified diffs Side-by-side diffs Delta from patch set Stats (+165 lines, -0 lines) Patch
M lib/typedObjects.js View 1 1 chunk +21 lines, -0 lines 0 comments Download
M lib/typedObjects/arrayTypes.js View 1 chunk +4 lines, -0 lines 0 comments Download
A lib/typedObjects/stringType.js View 1 1 chunk +63 lines, -0 lines 0 comments Download
M test/index.html View 1 chunk +1 line, -0 lines 0 comments Download
M test/tests/typedObjects.js View 1 chunk +76 lines, -0 lines 0 comments Download

Messages

Total messages: 6
Wladimir Palant
5 years, 6 months ago (2014-05-16 13:48:01 UTC) #1
René Jeschke
http://codereview.adblockplus.org/5665229014827008/diff/5629499534213120/lib/typedObjects.js File lib/typedObjects.js (right): http://codereview.adblockplus.org/5665229014827008/diff/5629499534213120/lib/typedObjects.js#newcode168 lib/typedObjects.js:168: * str4.get(0); // 111 - ASCII code of "o" ...
5 years, 6 months ago (2014-05-19 08:15:28 UTC) #2
tschuster
http://codereview.adblockplus.org/5665229014827008/diff/5629499534213120/lib/typedObjects/stringType.js File lib/typedObjects/stringType.js (right): http://codereview.adblockplus.org/5665229014827008/diff/5629499534213120/lib/typedObjects/stringType.js#newcode21 lib/typedObjects/stringType.js:21: I think a good idea would be to have ...
5 years, 6 months ago (2014-05-19 11:31:28 UTC) #3
René Jeschke
http://codereview.adblockplus.org/5665229014827008/diff/5629499534213120/lib/typedObjects/stringType.js File lib/typedObjects/stringType.js (right): http://codereview.adblockplus.org/5665229014827008/diff/5629499534213120/lib/typedObjects/stringType.js#newcode21 lib/typedObjects/stringType.js:21: On 2014/05/19 11:31:29, tschuster wrote: > I think a ...
5 years, 6 months ago (2014-05-19 13:55:00 UTC) #4
Wladimir Palant
Fixed the nits. Concerning internal storage of strings see below. http://codereview.adblockplus.org/5665229014827008/diff/5629499534213120/lib/typedObjects/stringType.js File lib/typedObjects/stringType.js (right): http://codereview.adblockplus.org/5665229014827008/diff/5629499534213120/lib/typedObjects/stringType.js#newcode21 ...
5 years, 6 months ago (2014-05-19 15:16:38 UTC) #5
René Jeschke
5 years, 6 months ago (2014-05-19 15:26:24 UTC) #6
LGTM

http://codereview.adblockplus.org/5665229014827008/diff/5629499534213120/lib/...
File lib/typedObjects/stringType.js (right):

http://codereview.adblockplus.org/5665229014827008/diff/5629499534213120/lib/...
lib/typedObjects/stringType.js:21: 
On 2014/05/19 15:16:39, Wladimir Palant wrote:
> Actually, a cstring type might make sense in some cases - but thinking about
the
> data we are usually dealing with, pretty much every string *could* have
> non-ASCII data in it. So simply using cstring in some places won't help us
much.
> If anything, we would need a string type that dynamically switches between
ASCII
> and UTF-16 but I couldn't think of any way to implement this without
introducing
> lots of complexity/overhead.
> 
> As to UTF-8, the overhead here was my main concern when I tested Emscripten.
It
> might be ok if most of the time you need strings to display them - but we want
> to do multiple pattern search and accessing individual characters efficiently
> was the main issue with implementing these algorithms in JavaScript.
> 
> To sum up, IMHO UTF-16 is good enough as a solution for now, I'm not aiming
for
> high optimization with the first version of this code (premature optimization
> and everything). We can still optimize things later.

Yep, right. Just wanted to hear if you already considered UTF-8 and didn't
address an immediate issue. (And, yep, the overhead for load/save would be quite
huge, also you loose the easy indexing into the string).
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5