kicad-developers team mailing list archive
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:
>>>>> I think the lines involving operator new() may blow up at runtime when the corresponding
>>>>> delete  is called.
>>>> 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.
>>> 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  < [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
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
>> 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.
> I can live with changing the comment to reflect the fact that no initialization is taking
> 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