kicad-developers team mailing list archive

[Dick Hollenbeck <dick@xxxxxxxxxxx>]

On 12/05/2011 12:13 PM, Wayne Stambaugh wrote:
> On 12/5/2011 11:55 AM, Dick Hollenbeck wrote:
>> On 12/05/2011 10:35 AM, Wayne Stambaugh wrote:
>>> On 12/5/2011 9:48 AM, Dick Hollenbeck wrote:
>>>> On 12/05/2011 08:02 AM, Wayne Stambaugh wrote:
>>>>> On 12/5/2011 12:30 AM, Dick Hollenbeck wrote:
>>>>>> Wayne,
>>>>>>
>>>>>> I think the lines involving operator new() may blow up at runtime when the corresponding
>>>>>> delete [] is called.
>>>>> Dick,
>>>>>
>>>>> I'm not using
>>>>>
>>>>> delete[] foo;        // This is not the same as delete foo[nn];
>>>>>
>>>>> where the contents of foo are allocated using
>>>>>
>>>>> foo = new();
>>>>>
>>>>> I'm using
>>>>>
>>>>> delete foo[nn];      // This is the same as tmp* = foo[nn]; delete tmp;
>>>>>
>>>>> where the contents of the array of foo pointers is assigned using
>>>>>
>>>>> foo[nn] = new( sizeof(foo) );
>>>>>
>>>>> In the case of m_BoardSide it is defined in the board class as
>>>>>
>>>>> MATRIX_CELL* m_BoardSide[MAX_SIDES_COUNT];
>>>>>
>>>>> so I'm just filling the array of pointers with the new() operator so delete()
>>>>> should work properly.
>>>>>
>>>>>> Have you tested this code?
>>>>> I ran the auto router and didn't it segfault and it generated the same results
>>>>> as the old code so it appears to be correct.
>>>>>
>>>>> Wayne
>>>> Ok.  A related concern on this line:
>>>>
>>>>         /* allocate Board & initialize everything to empty */
>>>>         m_BoardSide[kk] = (MATRIX_CELL*) operator new( ii * sizeof(MATRIX_CELL) );
>>>>
>>>>
>>>> My understanding is that the old code also did not run the constructor MATRIX_CELL on each
>>>> element within the range [0] < [ii-1].
>>> Yep.  I originally tried to use new[] but calling the constructor broke the
>>> auto router.  The original ZMalloc code did not call the constructor either so
>>> using new( count ) was the best short term solution.  I know it is not the most
>>> elegant solution but my main goal was to stop using malloc() because no one was
>>> testing if the result of the allocation was NULL.  At least new() will raise an
>>> assertion that will get caught further up the stack by wxWidgets.  It may still
>>> crash but a least you will know the the type of assertion that cause the
>>> problem even in release builds.
>> Your concept is sound.  I just wanted to point out that the behavior of the new code is
>> different, and that the comment seems out of date.  No initialization is taking place,
>> whereas before, there was.  The memory was being zeroed out.  This makes the false comment
>> even more dangerous, because it is misleading one into thinking something equivalent is
>> happening.
> Is this the comment?
>
> /* allocate Board & initialize everything to empty */
>
> I couldn't find anything else in the Doxygen comments about the memory being
> zeroed.  If this is the comment, I'll take out "initialize everything to empty"
> in my next commit.
>
> Wayne


/*
 * My memory allocation, memory space is cleared
 */
void* MyZMalloc( size_t nb_octets )
{
    void* pt_mem = MyMalloc( nb_octets );

    if( pt_mem )
        memset( pt_mem, 0, nb_octets );
    return pt_mem;
}


the above, from old common.cpp version 2224.


So I'm just saying that if it is your intent to initialize the memory to zeroes, then do
it.  If you are certain that the memory is fine NOT CLEARED TO ZEROES, then change the
comment, so somebody knows that something is different.  Because it is different.


Also there is a chance that you randomly got memory initialized to zeroes from your
operator new(), THIS TIME, in your testing, and got lucky.  But Joe KiCad user does not
get memory initialized to zeroes, and wishes he did.


Thanks

Dick