| Re: [eigen] portable reallocation... |
[ Thread Index |
Date Index
| More lists.tuxfamily.org/eigen Archives
]
- To: eigen@xxxxxxxxxxxxxxxxxxx
- Subject: Re: [eigen] portable reallocation...
- From: Hauke Heibel <hauke.heibel@xxxxxxxxxxxxxx>
- Date: Sat, 27 Feb 2010 17:40:49 +0100
- Dkim-signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=googlemail.com; s=gamma; h=domainkey-signature:mime-version:received:in-reply-to:references :date:message-id:subject:from:to:content-type :content-transfer-encoding; bh=ua/DSuh+3S9gyss5GiYpMMbC3yv5U+sWxnMc4+AK53I=; b=o/8EmDqF09U6JlSlYGkPWYu/i4UJLDrJWV/U7O7E/gaGkKdpPn0Ztasdpq7XPAipQS x3kV/9VTrSo9DaLAofSPIIg46T9iTn2nhP28DN7FDGJXoZEObfn+dHDxm4UdhFW+dp2D oIsgO2A2OH903StSz3RjDGNvcBA7qAmsz34Go=
- Domainkey-signature: a=rsa-sha1; c=nofws; d=googlemail.com; s=gamma; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :content-type:content-transfer-encoding; b=tgrejScxe/ah6E5XH0GW5ABQCOvvdHbVDSV3uHsPEXaIkPEKWzWwEIkeIjTINBbSVJ 82/FIYKuTHvJ0Z0B7w1mOXtoys/YBAnXK+LcjM4UGkqlmYlUOZv1HiCjP5ZCgqlaRScM GVGNY46LVk35q1Dfqi/xeB2FoZXpE7heHxLpI=
On Sat, Feb 27, 2010 at 4:26 PM, Benoit Jacob <jacob.benoit.1@xxxxxxxxx> wrote:
> You patch looks good globally.
>
> A few comments:
>
> 1)
>
> + // Comment from Hauke: IIRC, 'if (ptr)' is not required - you are
> free to call free(0)
> if(ptr)
> std::free(*(reinterpret_cast<void**>(ptr) - 1));
>
> but if ptr==0 then here we're dereferencing the 0 pointer, that's
> what's illegal. It's precisely to allow to do ei_aligned_free(0) that
> we have to do this if(ptr) here.
I should have seen the -1 ... it is crystal clear that this does not work.
> 2)
>
> Naming nitpicks:
>
> conservative_resize: I actually agree that it looks better than
> conservativeResize, but we have this convention about capitalization,
> and following it consistently has the merit of making it easier for
> the user to remember the names.
>
> eigen_mm_realloc: why not ei_mm_realloc ? like elsewhere in eigen.
>
> 3)
>
> Finally: is this well covered by existing unit tests? (Just checking)
All done and I think this test has full coverage.
- Hauke