kicad-developers team mailing list archive

[Wayne Stambaugh <stambaughw@xxxxxxxxxxx>]

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

> 
> 
>>> But that the Mzalloc() simply zeroed out the memory before returning it?
>>>
>>> I'm wondering if your comment is now accurate, since we:
>>>
>>> a) do not run the constructors, and
>>> b) do not zero out the memory.
>> Not zeroing out the memory would have been a problem if it was tested somewhere
>> in the code if used as an end of list indicator but is is not.  If it weren't
>> for the function SetCell() and this code it autoplace.cpp:
>>
>>     /* Initialize top layer. */
>>     if( Board.m_BoardSide[TOP] )
>>         memcpy( Board.m_BoardSide[TOP], Board.m_BoardSide[BOTTOM],
>>                 NbCells * sizeof(MATRIX_CELL) );
>>
>> then zeroing may have been an issue and I would have called memcpy() to zero it
>> out.  I can still do this if it poses a problem that I overlooked.  That's why
>> I ran the auto router on a board and looked for any visual difference in the
>> results.
>>
>> I'm guessing that this is some very old legacy C code that needs some serious
>> refactoring.  The fact that memcpy() is still in there makes me take pause.
>> There are multiple stand alone functions accessing the public member variables
>> in multiple classes and lots of other difficult to follow code paths in the
>> auto routing code.  Some day I'll revisit it along with a "few" other places in
>> Pcbnew that need some attention.
>>
>> Wayne
> 
> I can live with changing the comment to reflect the fact that no initialization is taking
> place.
> 
> Thanks,
> 
> Dick
> 
> 
> 
> 
> _______________________________________________
> Mailing list: https://launchpad.net/~kicad-developers
> Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
> Unsubscribe : https://launchpad.net/~kicad-developers
> More help   : https://help.launchpad.net/ListHelp
>