Hi Sukender, On Mon, Jan 11, 2010 at 8:59 AM, Sukender <[email protected]> wrote: > I'm affraid I disagree with your conclusions. During reading, the "new > Lib3dsVector[m->nfaces]" (ReaderWriter3DS.cpp, line 769) is a temporary > array, used to make lib3DS store computed normals (it is an array of lib3DS > structures). Those normals are then copied to OSG's ones (osg_normals array). > It's then a good idea to deallocate this temporary array, I guess! :)
Curious, when I did the original review it looked like you have removed a delete [] that was their originally. I must have made a mistake in xxdiff. > And ref_ptr<>s (as said in my previous mail) are a protection against > exceptions potentially thrown before objects are attached to the scene graph > (RAII). I understand this motivation and it's a reasonable one. It's not a critical issue though as it's only under very exceptional circumstances will such a leak occur, and in these circumstances there is a good chance that other code will crash anyway so I'd suggestion is a bit of hypothetical memory leak. My original reading of the submission had a delete [] being removed so I didn't want to merge a new leak for a fix of hypothetical fix... this original review was clearly flawed though... One issue that still stands in the code is that you haven't added any .get()'s for the new ref_ptr<> so the code won't compile when users compile the OSG ref_ptr<> without the automatic conversion of ref_ptr<T> to T*. A quick code reveals that the code is also still missing a delete []. I think the right thing to do would be to use an std::auto_ptr<> on the 3ds lib data you are creating rather than use a delete []. > Attached to this email are two osgDB_3DS files (against rev. 10937): > - ReaderWriter3DS.cpp, which contains both RAII and delete[] changes. > - WriterNodeVisitor.cpp, which contains a small fix about using non > 3-charcacters long file extensions (such as "jpeg", to be converted to > "jpg"). May this be useful elsewhere? > > Well, I hope it's okay! I've merged the changes to WriterNodeVisitor.cpp. Once we settle upon a final rev of ReaderWriter3DS.cpp that fixes all the leaks and will compile across the the board I'll merge these too. Cheers, Robert. _______________________________________________ osg-submissions mailing list [email protected] http://lists.openscenegraph.org/listinfo.cgi/osg-submissions-openscenegraph.org
