Introducing MemoryMappedExternalResource for creating an external...

Introducing MemoryMappedExternalResource for creating an external
resource from a memory mapped file for creating strings.

Committed as http://code.google.com/p/v8/source/detail?r=6400 and 6401

It was needed to fix types in platform-windows.cc and utils.cc to make it compilable under all supported platforms.

[mnaganov (inactive), 2011-01-16 10:16:02]

http://codereview.chromium.org/6240002/diff/1/src/platform-win32.cc
File src/platform-win32.cc (right):

http://codereview.chromium.org/6240002/diff/1/src/platform-win32.cc#newcode919
src/platform-win32.cc:919: Win32MemoryMappedFile(HANDLE file, HANDLE
file_mapping, void* memory,
Arg per line, please.

http://codereview.chromium.org/6240002/diff/1/src/platform-win32.cc#newcode922
src/platform-win32.cc:922: size_(size) { }
And init-per-line.

http://codereview.chromium.org/6240002/diff/1/src/utils.cc
File src/utils.cc (right):

http://codereview.chromium.org/6240002/diff/1/src/utils.cc#newcode280
src/utils.cc:280: : filename_(NULL), data_(NULL), length_(0),
Please either have a single line of initializations or init-per-line, not a mix.

http://codereview.chromium.org/6240002/diff/1/src/utils.cc#newcode282
src/utils.cc:282: 
Please remove this redundant empty line.

http://codereview.chromium.org/6240002/diff/1/src/utils.cc#newcode301
src/utils.cc:301: 
I this empty line and the one below a redundant.

http://codereview.chromium.org/6240002/diff/1/src/utils.cc#newcode342
src/utils.cc:342: const char* err_context = p - 10;
Please introduce a constant for this magic number.

http://codereview.chromium.org/6240002/diff/1/src/utils.cc#newcode351
src/utils.cc:351: PrintF(" after \"%s\"", buffer);
You can use the "precision" field to specify the maximum number of chars to
print, e.g. "%.10s".
To get the precision value from the args list, use the '*' char: "%.*s"

http://codereview.chromium.org/6240002/diff/1/src/v8utils.h
File src/v8utils.h (right):

http://codereview.chromium.org/6240002/diff/1/src/v8utils.h#newcode326
src/v8utils.h:326: explicit MemoryMappedExternalResource(const char* filename,
2-args constructor does not need to be explicit.

http://codereview.chromium.org/6240002/diff/1/src/v8utils.h#newcode333
src/v8utils.h:333: bool Exists() const { return file_ != NULL; }
Simple getter-like functions should be named lowercase.

http://codereview.chromium.org/6240002/diff/1/src/v8utils.h#newcode337
src/v8utils.h:337: bool EnsureIsAscii() const { return EnsureIsAscii(true); }
Not sure that both versions of EnsureIsAscii should exist.

[marklam, 2011-01-18 02:49:01]

http://codereview.chromium.org/6240002/diff/1/src/platform-win32.cc
File src/platform-win32.cc (right):

http://codereview.chromium.org/6240002/diff/1/src/platform-win32.cc#newcode919
src/platform-win32.cc:919: Win32MemoryMappedFile(HANDLE file, HANDLE
file_mapping, void* memory,
On 2011/01/16 10:16:02, Michail Naganov wrote:
> Arg per line, please.

Done.

http://codereview.chromium.org/6240002/diff/1/src/platform-win32.cc#newcode922
src/platform-win32.cc:922: size_(size) { }
On 2011/01/16 10:16:02, Michail Naganov wrote:
> And init-per-line.

Done.

http://codereview.chromium.org/6240002/diff/1/src/utils.cc
File src/utils.cc (right):

http://codereview.chromium.org/6240002/diff/1/src/utils.cc#newcode280
src/utils.cc:280: : filename_(NULL), data_(NULL), length_(0),
On 2011/01/16 10:16:02, Michail Naganov wrote:
> Please either have a single line of initializations or init-per-line, not a
mix.

Done.

http://codereview.chromium.org/6240002/diff/1/src/utils.cc#newcode282
src/utils.cc:282: 
On 2011/01/16 10:16:02, Michail Naganov wrote:
> Please remove this redundant empty line.

Done.

http://codereview.chromium.org/6240002/diff/1/src/utils.cc#newcode301
src/utils.cc:301: 
On 2011/01/16 10:16:02, Michail Naganov wrote:
> I this empty line and the one below a redundant.

Done.

http://codereview.chromium.org/6240002/diff/1/src/utils.cc#newcode342
src/utils.cc:342: const char* err_context = p - 10;
On 2011/01/16 10:16:02, Michail Naganov wrote:
> Please introduce a constant for this magic number.

Done.

http://codereview.chromium.org/6240002/diff/1/src/utils.cc#newcode351
src/utils.cc:351: PrintF(" after \"%s\"", buffer);
On 2011/01/16 10:16:02, Michail Naganov wrote:
> You can use the "precision" field to specify the maximum number of chars to
> print, e.g. "%.10s".
> To get the precision value from the args list, use the '*' char: "%.*s"

Done.

http://codereview.chromium.org/6240002/diff/1/src/v8utils.h
File src/v8utils.h (right):

http://codereview.chromium.org/6240002/diff/1/src/v8utils.h#newcode326
src/v8utils.h:326: explicit MemoryMappedExternalResource(const char* filename,
On 2011/01/16 10:16:02, Michail Naganov wrote:
> 2-args constructor does not need to be explicit.

Done.

http://codereview.chromium.org/6240002/diff/1/src/v8utils.h#newcode333
src/v8utils.h:333: bool Exists() const { return file_ != NULL; }
On 2011/01/16 10:16:02, Michail Naganov wrote:
> Simple getter-like functions should be named lowercase.

Done.

http://codereview.chromium.org/6240002/diff/1/src/v8utils.h#newcode337
src/v8utils.h:337: bool EnsureIsAscii() const { return EnsureIsAscii(true); }
On 2011/01/16 10:16:02, Michail Naganov wrote:
> Not sure that both versions of EnsureIsAscii should exist.

Done.  This was partially revised code to remove the use of default arguments. 
The default arg is not removed.

[mnaganov (inactive), 2011-01-18 10:13:34]

LGTM with comments addressed.

http://codereview.chromium.org/6240002/diff/15001/src/utils.cc
File src/utils.cc (right):

http://codereview.chromium.org/6240002/diff/15001/src/utils.cc#newcode301
src/utils.cc:301: if (file_) {
Please use explicit comparison: if (file_ != NULL).
Sorry, I missed this in the first round.

http://codereview.chromium.org/6240002/diff/15001/src/utils.cc#newcode313
src/utils.cc:313: if (file_) {
Ditto.

http://codereview.chromium.org/6240002/diff/15001/src/utils.cc#newcode329
src/utils.cc:329: if (c & 0x80) {
Ditto.

http://codereview.chromium.org/6240002/diff/15001/src/utils.cc#newcode352
src/utils.cc:352: if (err_context_length) {
Ditto.

http://codereview.chromium.org/6240002/diff/15001/src/utils.cc#newcode356
src/utils.cc:356: abort();
OS::Abort();

http://codereview.chromium.org/6240002/diff/15001/src/v8utils.h
File src/v8utils.h (right):

http://codereview.chromium.org/6240002/diff/15001/src/v8utils.h#newcode334
src/v8utils.h:334: bool IsEmpty() const { return length_ == 0; }
is_empty() -- this is a getter-like operation.

[marklam, 2011-01-18 17:32:47]

http://codereview.chromium.org/6240002/diff/15001/src/utils.cc
File src/utils.cc (right):

http://codereview.chromium.org/6240002/diff/15001/src/utils.cc#newcode301
src/utils.cc:301: if (file_) {
On 2011/01/18 10:13:34, Michail Naganov wrote:
> Please use explicit comparison: if (file_ != NULL).
> Sorry, I missed this in the first round.

Done.

http://codereview.chromium.org/6240002/diff/15001/src/utils.cc#newcode313
src/utils.cc:313: if (file_) {
On 2011/01/18 10:13:34, Michail Naganov wrote:
> Ditto.

Done.

http://codereview.chromium.org/6240002/diff/15001/src/utils.cc#newcode329
src/utils.cc:329: if (c & 0x80) {
On 2011/01/18 10:13:34, Michail Naganov wrote:
> Ditto.

Done.

http://codereview.chromium.org/6240002/diff/15001/src/utils.cc#newcode352
src/utils.cc:352: if (err_context_length) {
On 2011/01/18 10:13:34, Michail Naganov wrote:
> Ditto.

Done.

http://codereview.chromium.org/6240002/diff/15001/src/utils.cc#newcode356
src/utils.cc:356: abort();
On 2011/01/18 10:13:34, Michail Naganov wrote:
> OS::Abort();

Done.

http://codereview.chromium.org/6240002/diff/15001/src/v8utils.h
File src/v8utils.h (right):

http://codereview.chromium.org/6240002/diff/15001/src/v8utils.h#newcode334
src/v8utils.h:334: bool IsEmpty() const { return length_ == 0; }
On 2011/01/18 10:13:34, Michail Naganov wrote:
> is_empty() -- this is a getter-like operation.

Done.

[mnaganov (inactive), 2011-01-19 10:50:51]

Landed: http://code.google.com/p/v8/source/detail?r=6386

On 2011/01/18 17:32:47, marklam wrote:
> http://codereview.chromium.org/6240002/diff/15001/src/utils.cc
> File src/utils.cc (right):
> 
> http://codereview.chromium.org/6240002/diff/15001/src/utils.cc#newcode301
> src/utils.cc:301: if (file_) {
> On 2011/01/18 10:13:34, Michail Naganov wrote:
> > Please use explicit comparison: if (file_ != NULL).
> > Sorry, I missed this in the first round.
> 
> Done.
> 
> http://codereview.chromium.org/6240002/diff/15001/src/utils.cc#newcode313
> src/utils.cc:313: if (file_) {
> On 2011/01/18 10:13:34, Michail Naganov wrote:
> > Ditto.
> 
> Done.
> 
> http://codereview.chromium.org/6240002/diff/15001/src/utils.cc#newcode329
> src/utils.cc:329: if (c & 0x80) {
> On 2011/01/18 10:13:34, Michail Naganov wrote:
> > Ditto.
> 
> Done.
> 
> http://codereview.chromium.org/6240002/diff/15001/src/utils.cc#newcode352
> src/utils.cc:352: if (err_context_length) {
> On 2011/01/18 10:13:34, Michail Naganov wrote:
> > Ditto.
> 
> Done.
> 
> http://codereview.chromium.org/6240002/diff/15001/src/utils.cc#newcode356
> src/utils.cc:356: abort();
> On 2011/01/18 10:13:34, Michail Naganov wrote:
> > OS::Abort();
> 
> Done.
> 
> http://codereview.chromium.org/6240002/diff/15001/src/v8utils.h
> File src/v8utils.h (right):
> 
> http://codereview.chromium.org/6240002/diff/15001/src/v8utils.h#newcode334
> src/v8utils.h:334: bool IsEmpty() const { return length_ == 0; }
> On 2011/01/18 10:13:34, Michail Naganov wrote:
> > is_empty() -- this is a getter-like operation.
> 
> Done.