Switch out use of std::string and std::vector for large allocations for a buffer class that doesn...

Switch out use of std::string and std::vector for large allocations for a buffer class that doesn't throw exceptions.
The new buffer class is pretty simple and relies on the MemoryAllocator class that I previously to back large
allocations with mapped files when memory is scarce.  That reduced the number of crashes quite a bit but we
still crash on machines that are simply out of diskspace as well.  So, the right thing to do is to expect and
handle failures which is what this cl is all about.  What we should see once this has landed is that crash
dumps due to courgette running out of disk space should disappear from crash/ and instead we should see the
number of users that run into this particular problem in dashboards.

TEST=Courgette out-of-memory/out-of-diskspace errors should disappear from crash/
BUG=74777
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=80648

[grt (UTC plus 2), 2011-04-05 17:10:08]

Looks good.  It'll be nice to have proper dashboard reporting of the memory
problems rather than just crashdumps.

Could you update the copyright date in all files?

http://codereview.chromium.org/6677141/diff/1/courgette/assembly_program.cc
File courgette/assembly_program.cc (right):

http://codereview.chromium.org/6677141/diff/1/courgette/assembly_program.cc#n...
courgette/assembly_program.cc:172: if (slot == 0) {
use NULL rather than 0

http://codereview.chromium.org/6677141/diff/1/courgette/assembly_program.h
File courgette/assembly_program.h (right):

http://codereview.chromium.org/6677141/diff/1/courgette/assembly_program.h#ne...
courgette/assembly_program.h:85: Label* FindOrMakeAbs32Label(RVA rva);
Document that these may return NULL.

http://codereview.chromium.org/6677141/diff/1/courgette/assembly_program.h#ne...
courgette/assembly_program.h:108: CheckBool Emit(Instruction* instruction) {
This method is leaky.  Does Emit unconditionally take ownership of
|instruction|?  If so, it must delete it if push_back fails.  If not, callers
must delete the arg if Emit returns false.

http://codereview.chromium.org/6677141/diff/1/courgette/assembly_program.h#ne...
courgette/assembly_program.h:114: Label* FindLabel(RVA rva, RVAToLabel* labels);
Document that this may return NULL.

http://codereview.chromium.org/6677141/diff/1/courgette/assembly_program.h#ne...
courgette/assembly_program.h:123: Instruction** byte_instruction_cache_;
Consider ScopedVector

http://codereview.chromium.org/6677141/diff/1/courgette/encoded_program.h
File courgette/encoded_program.h (right):

http://codereview.chromium.org/6677141/diff/1/courgette/encoded_program.h#new...
courgette/encoded_program.h:40: CheckBool AddOrigin(RVA rva);
From what I can tell, if a call to any of the methods that return CheckBool here
returns false, the game is up and the whole EncodedProgram needs to be
discarded.  If this is correct, please document it as such.

http://codereview.chromium.org/6677141/diff/1/courgette/memory_allocator.h
File courgette/memory_allocator.h (right):

http://codereview.chromium.org/6677141/diff/1/courgette/memory_allocator.h#ne...
courgette/memory_allocator.h:97: bool PrepareTempFile(FilePath* path,
move |path| after |failure_handler|: out params come after in params.

http://codereview.chromium.org/6677141/diff/1/courgette/memory_allocator.h#ne...
courgette/memory_allocator.h:160: // An STL compatible memory allocator class
that allocates memory either
Since the allocate() method now returns NULL on error, this class template no
longer satisfies the requirements on allocator types specified in the standard. 
Please revise the comment accordingly and consider changing the name to
something like "NonThrowingAllocator" so that this is even more obvious.

http://codereview.chromium.org/6677141/diff/1/courgette/memory_allocator.h#ne...
courgette/memory_allocator.h:169: template<class T, AllocationFailureHandler
mem_failures_handler>
If you make this template<class T, AllocationFailureHandler mem_failures_handler
= NULL>, can you get rid of the #ifdef at the bottom of NoThrowBuffer?

Also, I don't see any users of MemoryAllocator that specify a non-NULL
mem_failures_handler.  Could you remove this entirely?

http://codereview.chromium.org/6677141/diff/1/courgette/memory_allocator.h#ne...
courgette/memory_allocator.h:287: class MemoryAllocator : public
std::allocator<T> {
This class template has different semantics than the OS_WIN version.  You
minimally need to override allocate() so that it returns NULL instead of
throwing std::bad_alloc.

http://codereview.chromium.org/6677141/diff/1/courgette/memory_allocator.h#ne...
courgette/memory_allocator.h:292: 
Add some documentation for this class and for its methods.

http://codereview.chromium.org/6677141/diff/1/courgette/memory_allocator.h#ne...
courgette/memory_allocator.h:293: template<typename T>
Consider:
template<typename T, class Allocator = MemoryAllocator<T> >

http://codereview.chromium.org/6677141/diff/1/courgette/memory_allocator.h#ne...
courgette/memory_allocator.h:297: const static size_t kAllocationFailure =
0xffffffff;
"static const" here and on the next line.

http://codereview.chromium.org/6677141/diff/1/courgette/memory_allocator.h#ne...
courgette/memory_allocator.h:298: const static size_t kStartSize = sizeof(T) >
0xff ? 1 : 0xff / sizeof(T);
Why 0xff rather than 0x100?

http://codereview.chromium.org/6677141/diff/1/courgette/memory_allocator.h#ne...
courgette/memory_allocator.h:320: CheckBool reserve(size_t size) {
Use WARN_UNUSED_RESULT (from base/compiler_specific.h) here and everywhere you
use CheckBool.  It doesn't do anything for MSVC at the moment, but it could be
made to do so in the future, in which case you'd have compile-time enforcement
of what CheckBool is doing.

http://codereview.chromium.org/6677141/diff/1/courgette/memory_allocator.h#ne...
courgette/memory_allocator.h:327: if (size < kStartSize) {
nit: remove braces

http://codereview.chromium.org/6677141/diff/1/courgette/memory_allocator.h#ne...
courgette/memory_allocator.h:350: 
if (size > alloc_.max_size() - size_)
  return false;

http://codereview.chromium.org/6677141/diff/1/courgette/memory_allocator.h#ne...
courgette/memory_allocator.h:354: if ((alloc_size_ - size_) < size) {
this code has a few problems with overflow.  the following is probably a bit
more verbose than needed, but i think covers the cases:
  if ((alloc_size_ - size_) < size) {
    const size_t max_size = alloc_.max_size();
    size_t new_size = alloc_size_ ? alloc_size_ : kStartSize;
    while (new_size < size_ + size) {
      if (new_size < max_size - new_size)
        new_size *= 2;
      else
        new_size = max_size;
    }
    if (!reserve(new_size))
      return false;
  }

http://codereview.chromium.org/6677141/diff/1/courgette/memory_allocator.h#ne...
courgette/memory_allocator.h:376: // It might simply be faster for us to don't
reallocate but only
nit: "It might be faster for us to simply change the size."

http://codereview.chromium.org/6677141/diff/1/courgette/memory_allocator.h#ne...
courgette/memory_allocator.h:422: return buffer_[index];
DCHECK(index < size_)?

http://codereview.chromium.org/6677141/diff/1/courgette/memory_allocator.h#ne...
courgette/memory_allocator.h:426: return buffer_[index];
DCHECK(index < size_)?

[tommi (sloooow) - chröme, 2011-04-05 19:06:53]

In addition to addressing comments I have added two installer files to the cl as
discussed.  Gcl is giving me upload problems right now but I'll give you a ping
as soon as the changes have been updated.

http://codereview.chromium.org/6677141/diff/1/courgette/assembly_program.cc
File courgette/assembly_program.cc (right):

http://codereview.chromium.org/6677141/diff/1/courgette/assembly_program.cc#n...
courgette/assembly_program.cc:172: if (slot == 0) {
On 2011/04/05 17:10:08, grt wrote:
> use NULL rather than 0

Done.

http://codereview.chromium.org/6677141/diff/1/courgette/assembly_program.h
File courgette/assembly_program.h (right):

http://codereview.chromium.org/6677141/diff/1/courgette/assembly_program.h#ne...
courgette/assembly_program.h:85: Label* FindOrMakeAbs32Label(RVA rva);
On 2011/04/05 17:10:08, grt wrote:
> Document that these may return NULL.

Done.

http://codereview.chromium.org/6677141/diff/1/courgette/assembly_program.h#ne...
courgette/assembly_program.h:108: CheckBool Emit(Instruction* instruction) {
On 2011/04/05 17:10:08, grt wrote:
> This method is leaky.  Does Emit unconditionally take ownership of
> |instruction|?  If so, it must delete it if push_back fails.  If not, callers
> must delete the arg if Emit returns false.

Done.

http://codereview.chromium.org/6677141/diff/1/courgette/assembly_program.h#ne...
courgette/assembly_program.h:114: Label* FindLabel(RVA rva, RVAToLabel* labels);
On 2011/04/05 17:10:08, grt wrote:
> Document that this may return NULL.

Done.

http://codereview.chromium.org/6677141/diff/1/courgette/assembly_program.h#ne...
courgette/assembly_program.h:123: Instruction** byte_instruction_cache_;
On 2011/04/05 17:10:08, grt wrote:
> Consider ScopedVector

changed to scoped_array as ScopedVector uses std::vector

http://codereview.chromium.org/6677141/diff/1/courgette/encoded_program.h
File courgette/encoded_program.h (right):

http://codereview.chromium.org/6677141/diff/1/courgette/encoded_program.h#new...
courgette/encoded_program.h:40: CheckBool AddOrigin(RVA rva);
On 2011/04/05 17:10:08, grt wrote:
> From what I can tell, if a call to any of the methods that return CheckBool
here
> returns false, the game is up and the whole EncodedProgram needs to be
> discarded.  If this is correct, please document it as such.

Done.

http://codereview.chromium.org/6677141/diff/1/courgette/memory_allocator.h
File courgette/memory_allocator.h (right):

http://codereview.chromium.org/6677141/diff/1/courgette/memory_allocator.h#ne...
courgette/memory_allocator.h:97: bool PrepareTempFile(FilePath* path,
On 2011/04/05 17:10:08, grt wrote:
> move |path| after |failure_handler|: out params come after in params.

Done.

http://codereview.chromium.org/6677141/diff/1/courgette/memory_allocator.h#ne...
courgette/memory_allocator.h:160: // An STL compatible memory allocator class
that allocates memory either
On 2011/04/05 17:10:08, grt wrote:
> Since the allocate() method now returns NULL on error, this class template no
> longer satisfies the requirements on allocator types specified in the
standard. 
> Please revise the comment accordingly and consider changing the name to
> something like "NonThrowingAllocator" so that this is even more obvious.

Done.

http://codereview.chromium.org/6677141/diff/1/courgette/memory_allocator.h#ne...
courgette/memory_allocator.h:169: template<class T, AllocationFailureHandler
mem_failures_handler>
On 2011/04/05 17:10:08, grt wrote:
> If you make this template<class T, AllocationFailureHandler
mem_failures_handler
> = NULL>, can you get rid of the #ifdef at the bottom of NoThrowBuffer?

Yes, but afaik the style guide doesn't allow that, right?

> Also, I don't see any users of MemoryAllocator that specify a non-NULL
> mem_failures_handler.  Could you remove this entirely?

Hmm... that's a good point.  My plan was to retain the option of easily going
back to the STL containers if we had problems.  However, I'm a bit uneasy with
this function pointer ugliness, so yes, I'll just get rid of it and accept the
fact that this is not STL anymore :)

http://codereview.chromium.org/6677141/diff/1/courgette/memory_allocator.h#ne...
courgette/memory_allocator.h:287: class MemoryAllocator : public
std::allocator<T> {
On 2011/04/05 17:10:08, grt wrote:
> This class template has different semantics than the OS_WIN version.  You
> minimally need to override allocate() so that it returns NULL instead of
> throwing std::bad_alloc.

That's intentional.  The problem I'm addressing is for Windows installations
specifically but retaining the previous functionality on other platforms.

http://codereview.chromium.org/6677141/diff/1/courgette/memory_allocator.h#ne...
courgette/memory_allocator.h:292: 
On 2011/04/05 17:10:08, grt wrote:
> Add some documentation for this class and for its methods.

Done.

http://codereview.chromium.org/6677141/diff/1/courgette/memory_allocator.h#ne...
courgette/memory_allocator.h:293: template<typename T>
On 2011/04/05 17:10:08, grt wrote:
> Consider:
> template<typename T, class Allocator = MemoryAllocator<T> >

Done.

http://codereview.chromium.org/6677141/diff/1/courgette/memory_allocator.h#ne...
courgette/memory_allocator.h:297: const static size_t kAllocationFailure =
0xffffffff;
On 2011/04/05 17:10:08, grt wrote:
> "static const" here and on the next line.

Done.

http://codereview.chromium.org/6677141/diff/1/courgette/memory_allocator.h#ne...
courgette/memory_allocator.h:298: const static size_t kStartSize = sizeof(T) >
0xff ? 1 : 0xff / sizeof(T);
On 2011/04/05 17:10:08, grt wrote:
> Why 0xff rather than 0x100?

Cause it's closer to fffffuuuuuuuu... :) no real reason, I'll change it to
0x100.

http://codereview.chromium.org/6677141/diff/1/courgette/memory_allocator.h#ne...
courgette/memory_allocator.h:320: CheckBool reserve(size_t size) {
On 2011/04/05 17:10:08, grt wrote:
> Use WARN_UNUSED_RESULT (from base/compiler_specific.h) here and everywhere you
> use CheckBool.  It doesn't do anything for MSVC at the moment, but it could be
> made to do so in the future, in which case you'd have compile-time enforcement
> of what CheckBool is doing.

Done.

http://codereview.chromium.org/6677141/diff/1/courgette/memory_allocator.h#ne...
courgette/memory_allocator.h:327: if (size < kStartSize) {
On 2011/04/05 17:10:08, grt wrote:
> nit: remove braces

Done.

http://codereview.chromium.org/6677141/diff/1/courgette/memory_allocator.h#ne...
courgette/memory_allocator.h:350: 
On 2011/04/05 17:10:08, grt wrote:
> if (size > alloc_.max_size() - size_)
>   return false;

Done.

http://codereview.chromium.org/6677141/diff/1/courgette/memory_allocator.h#ne...
courgette/memory_allocator.h:354: if ((alloc_size_ - size_) < size) {
On 2011/04/05 17:10:08, grt wrote:
> this code has a few problems with overflow.  the following is probably a bit
> more verbose than needed, but i think covers the cases:
>   if ((alloc_size_ - size_) < size) {
>     const size_t max_size = alloc_.max_size();
>     size_t new_size = alloc_size_ ? alloc_size_ : kStartSize;
>     while (new_size < size_ + size) {
>       if (new_size < max_size - new_size)
>         new_size *= 2;
>       else
>         new_size = max_size;
>     }
>     if (!reserve(new_size))
>       return false;
>   }

Done.

http://codereview.chromium.org/6677141/diff/1/courgette/memory_allocator.h#ne...
courgette/memory_allocator.h:376: // It might simply be faster for us to don't
reallocate but only
On 2011/04/05 17:10:08, grt wrote:
> nit: "It might be faster for us to simply change the size."

Done.

http://codereview.chromium.org/6677141/diff/1/courgette/memory_allocator.h#ne...
courgette/memory_allocator.h:422: return buffer_[index];
On 2011/04/05 17:10:08, grt wrote:
> DCHECK(index < size_)?

Done.

http://codereview.chromium.org/6677141/diff/1/courgette/memory_allocator.h#ne...
courgette/memory_allocator.h:426: return buffer_[index];
On 2011/04/05 17:10:08, grt wrote:
> DCHECK(index < size_)?

Done.

[tommi (sloooow) - chröme, 2011-04-05 21:50:15]

ok - changes uploaded. ptal.

On Tue, Apr 5, 2011 at 3:06 PM, <tommi@chromium.org> wrote:

> In addition to addressing comments I have added two installer files to the
> cl as
> discussed.  Gcl is giving me upload problems right now but I'll give you a
> ping
> as soon as the changes have been updated.
>
>
>
> http://codereview.chromium.org/6677141/diff/1/courgette/assembly_program.cc
> File courgette/assembly_program.cc (right):
>
>
>
http://codereview.chromium.org/6677141/diff/1/courgette/assembly_program.cc#n...
> courgette/assembly_program.cc:172: if (slot == 0) {
> On 2011/04/05 17:10:08, grt wrote:
>
>> use NULL rather than 0
>>
>
> Done.
>
>
> http://codereview.chromium.org/6677141/diff/1/courgette/assembly_program.h
> File courgette/assembly_program.h (right):
>
>
>
http://codereview.chromium.org/6677141/diff/1/courgette/assembly_program.h#ne...
> courgette/assembly_program.h:85: Label* FindOrMakeAbs32Label(RVA rva);
> On 2011/04/05 17:10:08, grt wrote:
>
>> Document that these may return NULL.
>>
>
> Done.
>
>
>
>
http://codereview.chromium.org/6677141/diff/1/courgette/assembly_program.h#ne...
> courgette/assembly_program.h:108: CheckBool Emit(Instruction*
> instruction) {
> On 2011/04/05 17:10:08, grt wrote:
>
>> This method is leaky.  Does Emit unconditionally take ownership of
>> |instruction|?  If so, it must delete it if push_back fails.  If not,
>>
> callers
>
>> must delete the arg if Emit returns false.
>>
>
> Done.
>
>
>
>
http://codereview.chromium.org/6677141/diff/1/courgette/assembly_program.h#ne...
> courgette/assembly_program.h:114: Label* FindLabel(RVA rva, RVAToLabel*
> labels);
> On 2011/04/05 17:10:08, grt wrote:
>
>> Document that this may return NULL.
>>
>
> Done.
>
>
>
>
http://codereview.chromium.org/6677141/diff/1/courgette/assembly_program.h#ne...
> courgette/assembly_program.h:123: Instruction** byte_instruction_cache_;
> On 2011/04/05 17:10:08, grt wrote:
>
>> Consider ScopedVector
>>
>
> changed to scoped_array as ScopedVector uses std::vector
>
>
> http://codereview.chromium.org/6677141/diff/1/courgette/encoded_program.h
> File courgette/encoded_program.h (right):
>
>
>
http://codereview.chromium.org/6677141/diff/1/courgette/encoded_program.h#new...
> courgette/encoded_program.h:40: CheckBool AddOrigin(RVA rva);
> On 2011/04/05 17:10:08, grt wrote:
>
>>  From what I can tell, if a call to any of the methods that return
>>
> CheckBool here
>
>> returns false, the game is up and the whole EncodedProgram needs to be
>> discarded.  If this is correct, please document it as such.
>>
>
> Done.
>
>
> http://codereview.chromium.org/6677141/diff/1/courgette/memory_allocator.h
> File courgette/memory_allocator.h (right):
>
>
>
http://codereview.chromium.org/6677141/diff/1/courgette/memory_allocator.h#ne...
> courgette/memory_allocator.h:97: bool PrepareTempFile(FilePath* path,
> On 2011/04/05 17:10:08, grt wrote:
>
>> move |path| after |failure_handler|: out params come after in params.
>>
>
> Done.
>
>
>
>
http://codereview.chromium.org/6677141/diff/1/courgette/memory_allocator.h#ne...
> courgette/memory_allocator.h:160: // An STL compatible memory allocator
> class that allocates memory either
> On 2011/04/05 17:10:08, grt wrote:
>
>> Since the allocate() method now returns NULL on error, this class
>>
> template no
>
>> longer satisfies the requirements on allocator types specified in the
>>
> standard.
>
>> Please revise the comment accordingly and consider changing the name
>>
> to
>
>> something like "NonThrowingAllocator" so that this is even more
>>
> obvious.
>
> Done.
>
>
>
>
http://codereview.chromium.org/6677141/diff/1/courgette/memory_allocator.h#ne...
> courgette/memory_allocator.h:169: template<class T,
> AllocationFailureHandler mem_failures_handler>
> On 2011/04/05 17:10:08, grt wrote:
>
>> If you make this template<class T, AllocationFailureHandler
>>
> mem_failures_handler
>
>> = NULL>, can you get rid of the #ifdef at the bottom of NoThrowBuffer?
>>
>
> Yes, but afaik the style guide doesn't allow that, right?
>
>
>  Also, I don't see any users of MemoryAllocator that specify a non-NULL
>> mem_failures_handler.  Could you remove this entirely?
>>
>
> Hmm... that's a good point.  My plan was to retain the option of easily
> going back to the STL containers if we had problems.  However, I'm a bit
> uneasy with this function pointer ugliness, so yes, I'll just get rid of
> it and accept the fact that this is not STL anymore :)
>
>
>
>
http://codereview.chromium.org/6677141/diff/1/courgette/memory_allocator.h#ne...
> courgette/memory_allocator.h:287: class MemoryAllocator : public
> std::allocator<T> {
> On 2011/04/05 17:10:08, grt wrote:
>
>> This class template has different semantics than the OS_WIN version.
>>
> You
>
>> minimally need to override allocate() so that it returns NULL instead
>>
> of
>
>> throwing std::bad_alloc.
>>
>
> That's intentional.  The problem I'm addressing is for Windows
> installations specifically but retaining the previous functionality on
> other platforms.
>
>
>
>
http://codereview.chromium.org/6677141/diff/1/courgette/memory_allocator.h#ne...
> courgette/memory_allocator.h:292:
> On 2011/04/05 17:10:08, grt wrote:
>
>> Add some documentation for this class and for its methods.
>>
>
> Done.
>
>
>
>
http://codereview.chromium.org/6677141/diff/1/courgette/memory_allocator.h#ne...
> courgette/memory_allocator.h:293: template<typename T>
> On 2011/04/05 17:10:08, grt wrote:
>
>> Consider:
>> template<typename T, class Allocator = MemoryAllocator<T> >
>>
>
> Done.
>
>
>
>
http://codereview.chromium.org/6677141/diff/1/courgette/memory_allocator.h#ne...
> courgette/memory_allocator.h:297: const static size_t kAllocationFailure
> = 0xffffffff;
> On 2011/04/05 17:10:08, grt wrote:
>
>> "static const" here and on the next line.
>>
>
> Done.
>
>
>
>
http://codereview.chromium.org/6677141/diff/1/courgette/memory_allocator.h#ne...
> courgette/memory_allocator.h:298: const static size_t kStartSize =
> sizeof(T) > 0xff ? 1 : 0xff / sizeof(T);
> On 2011/04/05 17:10:08, grt wrote:
>
>> Why 0xff rather than 0x100?
>>
>
> Cause it's closer to fffffuuuuuuuu... :) no real reason, I'll change it
> to 0x100.
>
>
>
>
http://codereview.chromium.org/6677141/diff/1/courgette/memory_allocator.h#ne...
> courgette/memory_allocator.h:320: CheckBool reserve(size_t size) {
> On 2011/04/05 17:10:08, grt wrote:
>
>> Use WARN_UNUSED_RESULT (from base/compiler_specific.h) here and
>>
> everywhere you
>
>> use CheckBool.  It doesn't do anything for MSVC at the moment, but it
>>
> could be
>
>> made to do so in the future, in which case you'd have compile-time
>>
> enforcement
>
>> of what CheckBool is doing.
>>
>
> Done.
>
>
>
>
http://codereview.chromium.org/6677141/diff/1/courgette/memory_allocator.h#ne...
> courgette/memory_allocator.h:327: if (size < kStartSize) {
> On 2011/04/05 17:10:08, grt wrote:
>
>> nit: remove braces
>>
>
> Done.
>
>
>
>
http://codereview.chromium.org/6677141/diff/1/courgette/memory_allocator.h#ne...
> courgette/memory_allocator.h:350:
> On 2011/04/05 17:10:08, grt wrote:
>
>> if (size > alloc_.max_size() - size_)
>>   return false;
>>
>
> Done.
>
>
>
>
http://codereview.chromium.org/6677141/diff/1/courgette/memory_allocator.h#ne...
> courgette/memory_allocator.h:354: if ((alloc_size_ - size_) < size) {
> On 2011/04/05 17:10:08, grt wrote:
>
>> this code has a few problems with overflow.  the following is probably
>>
> a bit
>
>> more verbose than needed, but i think covers the cases:
>>   if ((alloc_size_ - size_) < size) {
>>     const size_t max_size = alloc_.max_size();
>>     size_t new_size = alloc_size_ ? alloc_size_ : kStartSize;
>>     while (new_size < size_ + size) {
>>       if (new_size < max_size - new_size)
>>         new_size *= 2;
>>       else
>>         new_size = max_size;
>>     }
>>     if (!reserve(new_size))
>>       return false;
>>   }
>>
>
> Done.
>
>
>
>
http://codereview.chromium.org/6677141/diff/1/courgette/memory_allocator.h#ne...
> courgette/memory_allocator.h:376: // It might simply be faster for us to
> don't reallocate but only
> On 2011/04/05 17:10:08, grt wrote:
>
>> nit: "It might be faster for us to simply change the size."
>>
>
> Done.
>
>
>
>
http://codereview.chromium.org/6677141/diff/1/courgette/memory_allocator.h#ne...
> courgette/memory_allocator.h:422: return buffer_[index];
> On 2011/04/05 17:10:08, grt wrote:
>
>> DCHECK(index < size_)?
>>
>
> Done.
>
>
>
>
http://codereview.chromium.org/6677141/diff/1/courgette/memory_allocator.h#ne...
> courgette/memory_allocator.h:426: return buffer_[index];
> On 2011/04/05 17:10:08, grt wrote:
>
>> DCHECK(index < size_)?
>>
>
> Done.
>
>
> http://codereview.chromium.org/6677141/
>

[grt (UTC plus 2), 2011-04-06 02:46:38]

Would you be terribly annoyed if I asked you to use WARN_UNUSED_RESULT on all
methods that return CheckBool?

http://codereview.chromium.org/6677141/diff/6006/courgette/memory_allocator.h
File courgette/memory_allocator.h (right):

http://codereview.chromium.org/6677141/diff/6006/courgette/memory_allocator.h...
courgette/memory_allocator.h:102: __declspec(noinline)
Should this __declspec go away, too?

http://codereview.chromium.org/6677141/diff/6006/courgette/memory_allocator.h...
courgette/memory_allocator.h:119: // process' address space.
process's

http://codereview.chromium.org/6677141/diff/6006/courgette/memory_allocator.h...
courgette/memory_allocator.h:141: // implementation that throws those
exceptions.
should this line be removed?

[tommi (sloooow) - chröme, 2011-04-06 13:50:02]

not at all. done.

http://codereview.chromium.org/6677141/diff/6006/courgette/memory_allocator.h
File courgette/memory_allocator.h (right):

http://codereview.chromium.org/6677141/diff/6006/courgette/memory_allocator.h...
courgette/memory_allocator.h:102: __declspec(noinline)
On 2011/04/06 02:46:38, grt wrote:
> Should this __declspec go away, too?

Done.

http://codereview.chromium.org/6677141/diff/6006/courgette/memory_allocator.h...
courgette/memory_allocator.h:119: // process' address space.
On 2011/04/06 02:46:38, grt wrote:
> process's

Done.

http://codereview.chromium.org/6677141/diff/6006/courgette/memory_allocator.h...
courgette/memory_allocator.h:141: // implementation that throws those
exceptions.
On 2011/04/06 02:46:38, grt wrote:
> should this line be removed?

thanks!

[grt (UTC plus 2), 2011-04-06 17:27:47]

LGTM