Re: [eigen] portable reallocation...

[Benoit Jacob <jacob.benoit.1@xxxxxxxxx>]

2010/2/27 Benoit Jacob <jacob.benoit.1@xxxxxxxxx>:
> 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,

Rather the "-1" pointer. Which is just as bad :)

Benoit

> 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.
>
> 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)
>
> Benoit
>
>
> 2010/2/26 Hauke Heibel <hauke.heibel@xxxxxxxxxxxxxx>:
>> Finally the patch. It's a bunch of changes so I don't commit right away.
>>
>> Did I initially say it's easy... I was wrong. ;)
>>
>> Anyways, it seems to be working now. There is one warning coming from
>> GCC - is it possible to disable that per file?
>>
>> - Hauke
>>
>> On Fri, Feb 26, 2010 at 3:17 PM, Benoit Jacob <jacob.benoit.1@xxxxxxxxx> wrote:
>>> 2010/2/26 Hauke Heibel <hauke.heibel@xxxxxxxxxxxxxx>:
>>>> On Fri, Feb 26, 2010 at 12:48 PM, Benoit Jacob <jacob.benoit.1@xxxxxxxxx> wrote:
>>>>> Since this will only be used by Eigen, specifically by
>>>>> conservativeResize, your portable realloc wrapper doesn't have to have
>>>>> the standard realloc API where only the new size is passed. You can
>>>>> instead write your own realloc-like function that also takes the old
>>>>> size as an argument.
>>>>
>>>> Yes, I tried that. After implementing this, I stumbled over something
>>>> rather strange which we might want to discuss. :)
>>>>
>>>> When creating new memory we do
>>>>
>>>> template<typename T, bool Align> inline T*
>>>> ei_conditional_aligned_new(size_t size)
>>>> {
>>>>  T *result = reinterpret_cast<T*>(ei_conditional_aligned_malloc<Align>(sizeof(T)*size));
>>>>  return ei_construct_elements_of_array(result, size);
>>>> }
>>>>
>>>> For integral types the compiler seems to be able to remove
>>>> 'ei_construct_elements_of_array' but as soon as we try to allocate
>>>> e.g. std::complex arrays or user types T, the memory will be filled
>>>> with 'size' times 'new T()' -- why? Why do we initialize memory for
>>>> non integral types? I thought we agreed upon uninitialized matrix
>>>> creation!?
>>>
>>> We agreed to not initialized by zero.
>>>
>>> But we still have to call default constructors for each entry of the
>>> array. For example, imagine that the user is using Eigen with some
>>> multiple-precision numbers class as the Scalar type. Such a class
>>> would probably have a default constructor allocating some memory or at
>>> least initializing some pointers, even if it doesn't initialize
>>> anything by zero!
>>>
>>> So we are calling T() on each coefficient, but we're not initializing
>>> by zero (or initializing to any particular value at all).
>>>
>>> Then, if for some type T, T() initializes by zero, that's not our
>>> fault, it's the design choice of type T. At least that's not the case
>>> for int,float,double  (ints are not special in this respect). I don't
>>> know about std::complex but if they do initialize by zero then that's
>>> their own choice, not our fault.
>>>
>>>>
>>>> Just try
>>>>
>>>> VectorXcd m(50);
>>>> std::cout << m << std::endl;
>>>>
>>>> it will be all zeros - and not uninitialized objects.
>>>
>>> It's all zeros indeed, but that doesn't mean that they were
>>> initialized to zero. For example, I tried with VectorXi and I got
>>> zeros too: that means that the memory there happened to be filled with
>>> zeros.
>>>
>>> Benoit
>>>
>>>>
>>>> Maybe somebody has an idea?
>>>>
>>>> - Hauke
>>>>
>>>>
>>>>
>>>
>>>
>>>
>>
>