← Back to team overview

kicad-developers team mailing list archive

Re: operator new in board.cpp


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
>>>> 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.


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

Follow ups