From: Tor Didriksen
Date: April 28 2011 12:36pm
Subject: Re: bzr commit into mysql-trunk branch (tor.didriksen:3318) WL#5774
List-Archive: http://lists.mysql.com/commits/136298
Message-Id: <4DB95F6A.1030204@oracle.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit

On 2011-04-28 14:05, Roy Lyseng wrote:
> Hi Tor, Olav,
>
> like Olav, I do like this patch. I do however have a few comments 
> which I hope you will consider:
>
> 1. The mem_root member.
>
> I suggest that this member be made const, because I cannot see a 
> requirement to change mem_root during the lifetime of an object. In 
> fact, I think that we may be in for some nasty surprise if we do, and 
> it will at least constrain the way the mem_root allocator can be 
> implemented.
>
> It means that mem_root must be set in the constructor (I think that is 
> good), and you can get rid of the set_mem_root() function (really good).

done

>
> 2. Initial size, and lazy allocation
>
> If you take initsize as part of the template, you do not need to have 
> reserve() as an externally visible function, the initial space will be 
> set when the object is constructed. It may also be used to delay 
> allocation until the first insertion, which is slightly more efficient 
> for objects with zero final size.
>

For this case, we want initial size == 0, and only allocate memory if we 
actually need it.
I guess you are asking for "the size of the initial allocation if the 
initial size == 0"
I would prefer to keep the interface/semantics as close to std::vector 
as possible,
rather than introducing 'initial size' which really means 'initial size 
if we insert anything into the array'.

> 3. The name.
>
> I think that we should be able to come up with a better name than 
> Mem_root_array for the new class. The name should better reflect the 
> functionality that it provides, rather than being hung up with the 
> implementation detail of the mem_root. There may also after a while be 
> a whole set of such classes that rely on a dedicated memory allocator, 
> so they might need a good family name. Perhaps a namespace would we 
> good for this?
>
> Unfortunately I do not have a good name suggestion ;)

namespace memroot
{
class Array...
}

??

>
> Thanks,
> Roy