[serialization][1.33.0?] xml_iarchive leak
Robert, The following code leads to numerous leaks when lIn's contents are not an xml_archive. Any thoughts on the cause? try { std::ifstream lIn( lpszPathName ); if( lIn.good() ) { boost::archive::xml_iarchive ia( lIn ); } } catch( boost::archive::xml_archive_exception& aExc ) { std::string lErr = aExc.what(); } Thanks, Jeff Flinn
Hmm - this would surprise me. It looks to me that maybe I'm missing something about clean up from exceptions. baybe the std::ifstream lIn( lpszPathName ); should be moved outside the try block. I wouldn't think that should be necessary but now that I look at this it might be. What happens when you do this? Any C++ expert want to opine on this? Robert Ramey Jeff Flinn wrote:
Robert,
The following code leads to numerous leaks when lIn's contents are not an xml_archive. Any thoughts on the cause?
try { std::ifstream lIn( lpszPathName );
if( lIn.good() ) { boost::archive::xml_iarchive ia( lIn ); } } catch( boost::archive::xml_archive_exception& aExc ) { std::string lErr = aExc.what(); }
Thanks,
Jeff Flinn
Robert,
The following code leads to numerous leaks when lIn's contents are not an xml_archive. Any thoughts on the cause?
try { std::ifstream lIn( lpszPathName );
if( lIn.good() ) { boost::archive::xml_iarchive ia( lIn ); } } catch( boost::archive::xml_archive_exception& aExc ) { std::string lErr = aExc.what(); }
Hmm - this would surprise me. It looks to me that maybe I'm missing something about clean up from exceptions. baybe the std::ifstream lIn( lpszPathName ); should be moved outside the try block. I wouldn't think that should be necessary but now that I look at this it might be. What happens when you do this? Any C++ expert want to opine on this?
std::ifstream lIn( lpszPathName );
try { if( lIn.good() ) { boost::archive::xml_iarchive ia( lIn ); } } catch( boost::archive::xml_archive_exception& aExc ) { std::string lErr = aExc.what(); }
results in the same leaks. By the way this is with VC7.1. Jeff Flinn
"Robert Ramey" <ramey@rrsd.com> wrote in message news:ddiks0$mle$1@sea.gmane.org...
Hmm - this would surprise me. It looks to me that maybe I'm missing something about clean up from exceptions. baybe the std::ifstream lIn( lpszPathName ); should be moved outside the try block. I wouldn't think that should be necessary but now that I look at this it might be. What happens when you do this? Any C++ expert want to opine on this?
I took a quick look at the source for xml_iarchive. It happens that you use the Pimpl-idiom without using a smart pointer. Remember the destructor is only called if the constructor succeeds. This means that when you throw in the constructor you never execute "delete pimpl" in the destructor. I suggest that you use boost::scoped_ptr for all your pimpl classes. best regards Thorsten
Robert R. wrote [privately]
Originally I had used scoped_ptr there. The problem is that scoped_ptr isn't supported with all compilers - specifically Borland. I don't know about others.
If that is true then why do they pass the test??? (http://engineering.meta-comm.com/boost-regression/CVS-RC_1_33_0/user/smart_ ptr_release.html) If it is true scoped_ptr don't work with borland, then I suggest that you at least do template< class T > struct serialization_scoped_ptr { // extreamely simple stuff here }; best regards Thorsten
Thorsten Ottosen wrote:
Robert R. wrote [privately]
Originally I had used scoped_ptr there. The problem is that scoped_ptr isn't supported with all compilers - specifically Borland. I don't know about others.
If that is true then why do they pass the test???
(http://engineering.meta-comm.com/boost-regression/CVS-RC_1_33_0/user/smart_ ptr_release.html)
a) I don't see any tests of scoped_ptr there. b) If I recall correctly, the borland version of scoped_ptr fails when the type is incomplete - which makes it not useful for implementing the PIMPL idiom. c) I was reluctant to include the whole smart_ptr machinery for just this one case. I am surprised that the delete in the destructor fails to delete the pimpl. I've been unable to find the place where a constructor throws.
If it is true scoped_ptr don't work with borland, then I suggest that you at least do
template< class T > struct serialization_scoped_ptr { // extreamely simple stuff here };
I think the borland issue would still trip this up - scoped_ptr is very simple. Robert Ramey
"Robert Ramey" <ramey@rrsd.com> wrote in message news:ddqq76$9ng$1@sea.gmane.org...
If that is true then why do they pass the test???
(http://engineering.meta-comm.com/boost-regression/CVS-RC_1_33_0/user/smart_
ptr_release.html)
a) I don't see any tests of scoped_ptr there.
it is part of smart_ptr_test.cpp.
b) If I recall correctly, the borland version of scoped_ptr fails when the type is incomplete - which makes it not useful for implementing the PIMPL idiom.
it always fails unless the destructor is included first in the cpp file IIRC.
c) I was reluctant to include the whole smart_ptr machinery for just this one case.
ok.
I am surprised that the delete in the destructor fails to delete the pimpl. I've been unable to find the place where a constructor throws.
ok ... maybe the hypothesis can be checked by Jeff on a compiler where scoped_ptr does work? -Thorsten
"Thorsten Ottosen" <nesotto@cs.aau.dk> wrote in message news:ddqti0$jkh$1@sea.gmane.org...
"Robert Ramey" <ramey@rrsd.com> wrote in message news:ddqq76$9ng$1@sea.gmane.org...
If that is true then why do they pass the test???
(http://engineering.meta-comm.com/boost-regression/CVS-RC_1_33_0/user/smart_
ptr_release.html)
a) I don't see any tests of scoped_ptr there.
it is part of smart_ptr_test.cpp.
b) If I recall correctly, the borland version of scoped_ptr fails when the type is incomplete - which makes it not useful for implementing the PIMPL idiom.
it always fails unless the destructor is included first in the cpp file IIRC.
c) I was reluctant to include the whole smart_ptr machinery for just this one case.
ok.
I am surprised that the delete in the destructor fails to delete the pimpl. I've been unable to find the place where a constructor throws.
ok ... maybe the hypothesis can be checked by Jeff on a compiler where scoped_ptr does work?
The following change in xml_iarchive.hpp(along with removing the matching delete in the corresponding .ipp file) fixes the leaks: scoped_ptr<xml_grammar> gimpl; // xml_grammar *gimpl; Jeff Flinn
I think I've fixed this. I'll test it and check it in later. Thanks for spotting and finding this. Robert Ramery Jeff Flinn wrote:
"Thorsten Ottosen" <nesotto@cs.aau.dk> wrote in message news:ddqti0$jkh$1@sea.gmane.org...
"Robert Ramey" <ramey@rrsd.com> wrote in message news:ddqq76$9ng$1@sea.gmane.org...
If that is true then why do they pass the test???
(http://engineering.meta-comm.com/boost-regression/CVS-RC_1_33_0/user/smart_
ptr_release.html)
a) I don't see any tests of scoped_ptr there.
it is part of smart_ptr_test.cpp.
b) If I recall correctly, the borland version of scoped_ptr fails when the type is incomplete - which makes it not useful for implementing the PIMPL idiom.
it always fails unless the destructor is included first in the cpp file IIRC.
c) I was reluctant to include the whole smart_ptr machinery for just this one case.
ok.
I am surprised that the delete in the destructor fails to delete the pimpl. I've been unable to find the place where a constructor throws.
ok ... maybe the hypothesis can be checked by Jeff on a compiler where scoped_ptr does work?
The following change in xml_iarchive.hpp(along with removing the matching delete in the corresponding .ipp file) fixes the leaks:
scoped_ptr<xml_grammar> gimpl; // xml_grammar *gimpl;
Jeff Flinn
"Robert Ramey" <ramey@rrsd.com> wrote in message news:ddrgic$te8$1@sea.gmane.org...
I think I've fixed this. I'll test it and check it in later. Thanks for spotting and finding this.
Robert Ramery
I would appreciate if you considered using scoped_ptr...it should work for borland... if not then maybe you could use your own simple pointer. scoped_ptr also check via checked_delete() which is another benefit. br Thorsten
I originally used scoped_ptr but had to change it because it couldn't be made to work for borland. If someone would fix it so it works for borland, I would love to use it. Robert Ramey Thorsten Ottosen wrote:
"Robert Ramey" <ramey@rrsd.com> wrote in message news:ddrgic$te8$1@sea.gmane.org...
I think I've fixed this. I'll test it and check it in later. Thanks for spotting and finding this.
Robert Ramery
I would appreciate if you considered using scoped_ptr...it should work for borland... if not then maybe you could use your own simple pointer.
scoped_ptr also check via checked_delete() which is another benefit.
br
Thorsten
Robert Ramey wrote:
I originally used scoped_ptr but had to change it because it couldn't be made to work for borland. If someone would fix it so it works for borland, I would love to use it.
I don't think scoped_ptr will ever work with Borland due to compiler bugs. Could a #ifdef change it to a shared_ptr just for Borland compilers? Not ideal but would work I assume. Cheers Russell
The current fix isn't ideal but does work (I assume) as well - and it doesn't drag in shared_ptr. So I can't see a case for changing it. Robert Ramey Russell Hind wrote:
Robert Ramey wrote:
I originally used scoped_ptr but had to change it because it couldn't be made to work for borland. If someone would fix it so it works for borland, I would love to use it.
I don't think scoped_ptr will ever work with Borland due to compiler bugs. Could a #ifdef change it to a shared_ptr just for Borland compilers? Not ideal but would work I assume.
Cheers
Russell
"Robert Ramey" <ramey@rrsd.com> wrote in message news:de2fhj$nj3$1@sea.gmane.org...
The current fix isn't ideal but does work (I assume) as well - and it doesn't drag in shared_ptr. So I can't see a case for changing it.
is this fix to use scoped_ptr for normal systems and a hack for borland? -Thorsten
participants (4)
-
Jeff Flinn -
Robert Ramey -
Russell Hind -
Thorsten Ottosen