Re: [eigen] portable reallocation...

[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, 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
>>>
>>>
>>>
>>
>>
>>
>