About erasing iterators (ohierarchyholder.cxx, package module)

[Jan Holesovsky]

Hi Julien,

Julien Nabet píše v St 28. 08. 2013 v 08:43 +0200:

> >      289 void OHierarchyElement_Impl::RemoveElement( const ::rtl::Reference<
> > OHierarchyElement_Impl>&  aRef )
> >      290 {
> >      291     {
> >      292         ::osl::MutexGuard aGuard( m_aMutex );
> >      293         OHierarchyElementList_Impl::iterator aIter =
> > m_aChildren.begin();
> >      294         const OHierarchyElementList_Impl::const_iterator aEnd =
> > m_aChildren.end();
> >      295         while (aIter != aEnd)
> >      296         {
> >      297             if (aIter->second == aRef )
> >      298                 aIter = m_aChildren.erase(aIter);
> >      299             else
> >      300                 ++aIter;
> >      301         }
> >      302     }
> > See
> > http://opengrok.libreoffice.org/xref/core/package/source/xstor/ohierarchyholder.cxx#298
> >
> > Is it ok to use "aEnd" or, since erase may  be called, we should change the
> > while into:
> > while (aIter != m_aChildren.end())
> > (and remove aEnd)
> > In this exact case (when the value may be present more times in the
> > vector), you might want to use the Erase-remove idiom [1]:
> >
> > // remove all occurrences of aRef
> > m_aChildren.erase(std::remove(m_aChildren.begin(), m_aChildren.end(), aRef), m_aChildren.end());
> >
> > Even with the comment I suppose, so that people who haven't read tons of
> > C++ books can see what's going on ;-)
> I copy pasted this exact line and it failed to compile :-(

Ah, sorry!  I did not notice m_aChildren is not a sequence, but an
unordered_map :-( - so you cannot use this.

> >> Another thing: couldn't we break the loop after erase or could aRef be
> >> present several times?
> > Not sure - worth checking the history of that file I think.
> According to git history, it's been like this since first git log, so 
> let's let this.
> About end iterator, it was my fault since I had created a const end 
> iterator in the beginning of this year. I've fixed this (see 
> http://cgit.freedesktop.org/libreoffice/core/commit/?id=9ebcd5ec54ec5d77cf46849f7f00bf915644f6e1)

If I am not mistaken, unordered_map is a _Hashtable, with end() defined
as const_iterator(nullptr), so it is constant, and your change was
actually fine.  Then again, I am no C++ expert, so probably better to be
safe, and just check against m_aChildren.end() ;-) - you don't save much
anyway, .end() is cheap.

All the best,
Kendy