Issue 11443005: Issue - 7123

Issue 11443005: Issue - 7123 (Closed)

Created:
8 years ago by siva

Modified:
8 years ago

Reviewers:
cshapiro

CC:
reviews_dartlang.org, vm-dev_dartlang.org

Base URL:
http://dart.googlecode.com/svn/branches/bleeding_edge/dart/runtime/

Visibility:
Public.

More Reviews

Description

Issue - 7123 Rename String::New(const uint8_t latin1_array......) to String::FromLatin1 String::New(const uint16_t utf16_array......) to String::FromUTF16 String::New(const int32_t utf32_array.......) to String::FromUTF32 Committed: https://code.google.com/p/dart/source/detail?r=15763

Patch Set 1 #

Patch Set 2 : #

Patch Set 3 : #

Total comments: 12

Patch Set 4 : #

Created: 8 years ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+73 lines, -61 lines)			Patch
M	vm/dart_api_impl.cc	View	1 2 3	2 chunks	+2 lines, -2 lines	0 comments	Download
M	vm/object.h	View	1 2 3	2 chunks	+11 lines, -13 lines	0 comments	Download
M	vm/object.cc	View	1 2 3	2 chunks	+9 lines, -9 lines	0 comments	Download
M	vm/object_test.cc	View	1 2 3	15 chunks	+19 lines, -19 lines	0 comments	Download
M	vm/raw_object_snapshot.cc	View	1 2 3	4 chunks	+5 lines, -4 lines	0 comments	Download
M	vm/symbols.h	View	1 2 3	1 chunk	+4 lines, -2 lines	0 comments	Download
M	vm/symbols.cc	View	1 2 3	4 chunks	+23 lines, -12 lines	0 comments	Download

Messages

Total messages: 3 (0 generated)

Expand Messages | Collapse Messages

cshapiro

lgtm with a few comments https://codereview.chromium.org/11443005/diff/9001/vm/object.h File vm/object.h (right): https://codereview.chromium.org/11443005/diff/9001/vm/object.h#newcode3930 vm/object.h:3930: RawString* (*NewSymbol)(const ElementType* chars, ...

8 years ago (2012-12-05 20:40:16 UTC) #2

siva

8 years ago (2012-12-05 23:33:14 UTC) #3

https://codereview.chromium.org/11443005/diff/9001/vm/object.h
File vm/object.h (right):

https://codereview.chromium.org/11443005/diff/9001/vm/object.h#newcode3930
vm/object.h:3930: RawString* (*NewSymbol)(const ElementType* chars,
On 2012/12/05 20:40:16, cshapiro wrote:
> You can greatly simplify this by having the callback declared as a template
> parameter
> 
>   template<typename HandleType, typename ElementType, typename CallbackType>
>   static void ReadFromImpl(...
>                            CallbackType new_symbol,
>                            ...)
> 
> Either way, NewSymbol should be new_symbol since it is an argument.

Done.

https://codereview.chromium.org/11443005/diff/9001/vm/raw_object_snapshot.cc
File vm/raw_object_snapshot.cc (right):

https://codereview.chromium.org/11443005/diff/9001/vm/raw_object_snapshot.cc#...
vm/raw_object_snapshot.cc:1610: RawString* (*NewSymbol)(const CharacterType*
chars,
On 2012/12/05 20:40:16, cshapiro wrote:
> s/NewSymbol/new_symbol/
> 
> Also, see my comment in the header regarding the type declaration.

Done.

https://codereview.chromium.org/11443005/diff/9001/vm/symbols.cc
File vm/symbols.cc (right):

https://codereview.chromium.org/11443005/diff/9001/vm/symbols.cc#newcode146
vm/symbols.cc:146: RawString* (*New)(const T* chars,
On 2012/12/05 20:40:16, cshapiro wrote:
> rename New, possibly to new_string

Done.

https://codereview.chromium.org/11443005/diff/9001/vm/symbols.cc#newcode181
vm/symbols.cc:181: RawString* (*New)(const uint8_t* chars,
On 2012/12/05 20:40:16, cshapiro wrote:
> ditto on the rename here and in the two cases below

Done.

https://codereview.chromium.org/11443005/diff/9001/vm/symbols.h
File vm/symbols.h (right):

https://codereview.chromium.org/11443005/diff/9001/vm/symbols.h#newcode218
vm/symbols.h:218: template<typename T>
On 2012/12/05 20:40:16, cshapiro wrote:
> I would rename T to CharacterType and add a CallbackType parameter to replace
> the New signature below or, create a typedef for it.

Done.

https://codereview.chromium.org/11443005/diff/9001/vm/symbols.h#newcode221
vm/symbols.h:221: RawString* (*New)(const T* chars,
On 2012/12/05 20:40:16, cshapiro wrote:
> s/New/new_string/
> 
> (Can't call it new because that is a reserved word, need to make it lower case
> to match the style guide.)
> 

Done.

Expand Messages | Collapse Messages