I was doing what I do regularly with std::maps... a m_plsts.erase( m_plsts.begin(), m_plsts.end() ); on a boost::ptr_map. It failed with an assertion error... Digging around, it came from ptr_container/detail/associative_ptr_container.hpp on line 117. It appears that every single erase (including that done by a key) has an assert for the list not being empty! Why is this the case? As far as I understand, it is valid to attempt to remove a key that does not exist. It follows that the same behaviour would occur whether the map was empty or not. Attempting to remove an element from an empty map should not result in an abort. If I'm unaware of some C++ convention I would love to be informed, however this definitely screams bug to me. Joshua Moore-Oliva
Joshua Moore-Oliva wrote:
I was doing what I do regularly with std::maps... a m_plsts.erase( m_plsts.begin(), m_plsts.end() ); on a boost::ptr_map. It failed with an assertion error... Digging around, it came from ptr_container/detail/associative_ptr_container.hpp on line 117. It appears that every single erase (including that done by a key) has an assert for the list not being empty!
Why is this the case? As far as I understand, it is valid to attempt to remove a key that does not exist. It follows that the same behaviour would occur whether the map was empty or not. Attempting to remove an element from an empty map should not result in an abort.
If I'm unaware of some C++ convention I would love to be informed, however this definitely screams bug to me.
I think you're right. Thomas, is it ok to fix this in the 1.34 branch? -Thorsten
Thorsten, Thorsten Ottosen wrote:
Joshua Moore-Oliva wrote:
If I'm unaware of some C++ convention I would love to be informed, however this definitely screams bug to me.
I think you're right.
Thomas, is it ok to fix this in the 1.34 branch?
Sounds like it needs fixing, can you post a patch first? Thomas -- Thomas Witt witt@acm.org
Thomas Witt wrote:
Thorsten,
Thorsten Ottosen wrote:
Joshua Moore-Oliva wrote:
If I'm unaware of some C++ convention I would love to be informed, however this definitely screams bug to me.
I think you're right.
Thomas, is it ok to fix this in the 1.34 branch?
This was a non-issue for 1.34.
Sounds like it needs fixing, can you post a patch first?
However, I discovered that erase() needed to be overloaded for a Range argument. The patch is shown below (additionally two test files needs to be slightly opdated. Let me know if I can commit those changes. -Thorsten Index: detail/associative_ptr_container.hpp =================================================================== RCS file: /cvsroot/boost/boost/boost/ptr_container/detail/associative_ptr_contai ner.hpp,v retrieving revision 1.8 diff -u -r1.8 associative_ptr_container.hpp --- detail/associative_ptr_container.hpp 18 Feb 2006 01:41:14 -00001.8 +++ detail/associative_ptr_container.hpp 2 Jan 2007 18:13:46 -0000 @@ -121,6 +121,12 @@ this->c_private().erase( first.base(), last.base() );// nothrow return res; // nothrow } + + template< class Range > + iterator erase( const Range& r ) + { + return erase( boost::begin(r), boost::end(r) ); + } protected:
Thorsten Ottosen wrote:
Joshua Moore-Oliva wrote:
I was doing what I do regularly with std::maps... a m_plsts.erase( m_plsts.begin(), m_plsts.end() ); on a boost::ptr_map. It failed with an assertion error... Digging around, it came from ptr_container/detail/associative_ptr_container.hpp on line 117. It appears that every single erase (including that done by a key) has an assert for the list not being empty!
I could not see that assertion in the 1.34 RC branch? Are your examples from 1.33? Here's the definition from 1.34: size_type erase( const key_type& x ) // nothrow { iterator i( this->c_private().find( x ) ); // nothrow if( i == this->end() ) // nothrow return 0u; // nothrow this->remove( i ); // nothrow return this->c_private().erase( x ); // nothrow } This works fine if the map is empty. Wheter the overload that takes an iterator should be allowed to accept the end iterator is another issue. I'll give it some thought. -Thorsten
----- Mensaje original ----- De: Thorsten Ottosen <thorsten.ottosen@dezide.com> Fecha: Martes, Enero 2, 2007 6:44 pm Asunto: Re: [boost] Weird behaviour/bug in ptr_map Para: boost@lists.boost.org
Thorsten Ottosen wrote:
Joshua Moore-Oliva wrote: [...] It appears that every single erase (including that done by a key) has an assert for the list not being empty!
I could not see that assertion in the 1.34 RC branch? Are your examples from 1.33?
Here's the definition from 1.34:
size_type erase( const key_type& x ) // nothrow { iterator i( this->c_private().find( x ) ); // nothrow if( i == this->end() ) // nothrow return 0u; // nothrow this->remove( i ); // nothrow return this->c_private().erase( x ); // nothrow }
This works fine if the map is empty.
Hello Thorsten, this issue was already raised and corrected on September 2005, you might want to consult the thread at http://tinyurl.com/tpl54 .
Wheter the overload that takes an iterator should be allowed to accept the end iterator is another issue. I'll give it some thought.
My humble opinion is that you shouldn't allow an an end iterator to be legally passed to erase(iterator), that is, your current BOOST_ASSERT( before != this->end() ); is conformig. The rational is that erase(end()) is illegal, or at least undefined behavior, in STL containers. The additional check BOOST_ASSERT( !this->empty() ); at the same member function is almost immaterial, since if the container is empty then end() is the only valid iterator that can be possibly passed in. Best, Joaquín M López Muñoz Telefónica, Investigación y Desarrollo
My humble opinion is that you shouldn't allow an an end iterator to be legally passed to erase(iterator), that is, your current
Thinking about it, I would agree with you. I have some editing to do :D However, the empty asserts were still in 1.33 on erase with a key, but that issue seems to be resolved when the next boost comes out.
BOOST_ASSERT( before != this->end() );
is conformig. The rational is that erase(end()) is illegal, or at least undefined behavior, in STL containers.
The additional check
BOOST_ASSERT( !this->empty() );
at the same member function is almost immaterial, since if the container is empty then end() is the only valid iterator that can be possibly passed in.
But it's still material for erase( key ). Josh
Best,
Joaquín M López Muñoz Telefónica, Investigación y Desarrollo _______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
JOAQUIN LOPEZ MU?Z wrote:
Hello Thorsten, this issue was already raised and corrected on September 2005, you might want to consult the thread at http://tinyurl.com/tpl54 .
Thanks for the hint! :-)
Wheter the overload that takes an iterator should be allowed to accept the end iterator is another issue. I'll give it some thought.
My humble opinion is that you shouldn't allow an an end iterator to be legally passed to erase(iterator), that is, your current
BOOST_ASSERT( before != this->end() );
is conformig. The rational is that erase(end()) is illegal, or at least undefined behavior, in STL containers.
Is it? I couldn't easily deduce that from the standard.
The additional check
BOOST_ASSERT( !this->empty() );
at the same member function is almost immaterial, since if the container is empty then end() is the only valid iterator that can be possibly passed in.
Well, it doesn't really hurt, and it might catch error where an iterator from one container is used with another container. cheers -Thorsten
----- Mensaje original ----- De: Thorsten Ottosen <thorsten.ottosen@dezide.com> Fecha: Miércoles, Enero 3, 2007 12:38 pm Asunto: Re: [boost] Weird behaviour/bug in ptr_map Para: boost@lists.boost.org
JOAQUIN LOPEZ MU?Z wrote:
My humble opinion is that you shouldn't allow an an end iterator to be legally passed to erase(iterator), that is, your current
BOOST_ASSERT( before != this->end() );
is conformig. The rational is that erase(end()) is illegal, or at least undefined behavior, in STL containers.
Is it? I couldn't easily deduce that from the standard.
If you go to 23.1.2/7 where a table is given with requirements for associative containers, the expression "a.erase(q)" involves the iterator q, which is previously defined to represent a valid *dereferenceable* iterator to a --and end() is not dereferenceable. Analogous conditions are present for sequence containers. Joaquín M López Muñoz Telefónica, Investigación y Desarrollo
JOAQUIN LOPEZ MU?Z wrote:
is conformig. The rational is that erase(end()) is illegal, or at least undefined behavior, in STL containers.
Is it? I couldn't easily deduce that from the standard.
If you go to 23.1.2/7 where a table is given with requirements for associative containers, the expression "a.erase(q)" involves the iterator q, which is previously defined to represent a valid *dereferenceable* iterator to a --and end() is not dereferenceable. Analogous conditions are present for sequence containers.
Thanks. So obvious :-) -Thorsten
participants (4)
-
"JOAQUIN LOPEZ MU?Z" -
Joshua Moore-Oliva -
Thomas Witt -
Thorsten Ottosen