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 : [email protected] > Unsubscribe : https://launchpad.net/~kicad-developers > More help : https://help.launchpad.net/ListHelp > _______________________________________________ Mailing list: https://launchpad.net/~kicad-developers Post to : [email protected] Unsubscribe : https://launchpad.net/~kicad-developers More help : https://help.launchpad.net/ListHelp
