About b3dpolygon.cxx (basegfx)

[Markus Mohrhard]

Hey julien,


2013/4/1 julien2412 <serval2412 at yahoo.fr>

> Hello,
>
> There are a lot of iterators stuff in
> core/basegfx/source/polygon/b3dpolygon.cxx, I tried to fix 'prefix ++/--
> operators for non-primitive types' errors (provided by cppcheck) but I saw
> several iterator things I'm not sure, eg:
>     274     BColorArray(const BColorArray& rOriginal, sal_uInt32 nIndex,
> sal_uInt32 nCount)
>     275     :   maVector(),
>     276         mnUsedEntries(0L)
>     277     {
>     278         BColorDataVector::const_iterator
> aStart(rOriginal.maVector.begin());
>     279         aStart += nIndex;
>     280         BColorDataVector::const_iterator aEnd(aStart);
>     281         aEnd += nCount;
>     282         maVector.reserve(nCount);
>     283
>     284         for(; aStart != aEnd; ++aStart)
>     285         {
>     286             if(!aStart->equalZero())
>     287                 mnUsedEntries++;
>     288
>     289             maVector.push_back(*aStart);
>     290         }
>     291     }
> Isn't aEnd invalidated by push_back call?
>

No. Push_back only invalidates if it has to reallocate the memory for the
elements which does not happen if you make sure that enough space is
reserved. Additionally we have two different vectors here and therefore we
have no invalidation in any case.


Isn't aEnd += nCount dangerous? (if aEnd tries to go further than
> rOriginal.maVector.end())
>

It very much depends on what nCount is. If nCount is always <=
rOriginal.maVector.size then this call is perfectly safe. I would add an
assert statement into this method to make sure that the assumption is
always true.


>
>     358     void insert(sal_uInt32 nIndex, const BColorArray& rSource)
>     359     {
>     360         const sal_uInt32 nCount(rSource.maVector.size());
>     361
>     362         if(nCount)
>     363         {
>     364             // insert data
>     365             BColorDataVector::iterator aIndex(maVector.begin());
>     366             aIndex += nIndex;
>     367             BColorDataVector::const_iterator
> aStart(rSource.maVector.begin());
>     368             BColorDataVector::const_iterator
> aEnd(rSource.maVector.end());
>     369             maVector.insert(aIndex, aStart, aEnd);
>     370
>     371             for(; aStart != aEnd; ++aStart)
>     372             {
>     373                 if(!aStart->equalZero())
>     374                     mnUsedEntries++;
>     375             }
>     376         }
>     377     }
>
> Isn't aEnd iterator invalidated by insert call?
>

No. Notice that there are two different vectors involved. rSource.maVector
which provides aStart and aEnd is not changed. This is just the range
version of the the insert method that insert all elements between aStart
and aEnd into maVector.


>
> There are others in this file but again, I'm not sure, I'm just wondering.
>

Except for the case with nCount which depends on the context these cases
are correct. The way forward is to add an assert statement to make sure
that this assumption is true and check all the callers.

Regards,
Markus
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/libreoffice/attachments/20130401/f9db47c4/attachment.html>