[poppler] poppler/Array.cc poppler/Dict.cc

[Albert Astals Cid]

El dimarts, 8 de maig de 2018, a les 19:26:57 CEST, Adam Reichold va escriure:
> Hello again,
> 
> sorry for the rapid succession of patches.
> 
> The attached version fixes a linker issue due to non-exported weak
> symbols and fixes all users of Dict::add to not call copyString 

The behaviour change in Dict:add is really bad, i know people are not supposed to use the internal headers, but they still do, and i've no idea how many leaks we would cause if we merge this.

Any idea how to prevent it other than doing a source incompatible change (i.e renaming add to insert) so at least their code stops compiling and maybe they look at the differences?

Or can we even maybe keep the old behaviour?

Cheers,
  Albert

> and leak
> memory since std::string will take care of this including SBO.
> 
> Best regards, Adam.
> 
> P.S.: Will try to do some performance comparisons against master but
> this will take at least a day due to the limitations of my machine.
> 
> Am 08.05.2018 um 08:23 schrieb Adam Reichold:
> > Also cleaned up the includes a bit...
> > 
> > Am 08.05.2018 um 08:01 schrieb Adam Reichold:
> >> Hello again again,
> >>
> >> Am 08.05.2018 um 07:37 schrieb Adam Reichold:
> >>> Hello again,
> >>>
> >>> Am 08.05.2018 um 00:12 schrieb Albert Astals Cid:
> >>>> El dilluns, 7 de maig de 2018, a les 23:03:00 CEST, Adam Reichold va escriure:
> >>>>> Hello,
> >>>>>
> >>>>> it is probably already too much purely technical code churn for this
> >>>>> release cycle, but the use of memmove got me thinking how an
> >>>>> implementation of Dict with more STL usage would look like.
> >>>>>
> >>>>> The result is attached, requesting comments on whether this is
> >>>>> considered useful or not.
> >>>>
> >>>> The remove implementation is wrong since it breaks sorting.
> >>>
> >>> Could you explain in more detail? My thinking was that any sublist of a
> >>> sorted list is also sorted and hence removing an element of a list
> >>> cannot break its sorting? (And since we do not increase the size by
> >>> removing an element, we cannot reach SORT_LENGTH_LOWER_LIMIT if we were
> >>> not there already.)
> >>
> >> Sorry, that took a minute... Because of the swap... But actually, there
> >> already is std::vector::erase which does all the tricks like using
> >> memmove if it is statically safe, so it should actually be simpler.
> >>
> >> Fixed version attached...
> >>
> >>> Best regards,
> >>> Adam
> >>>
> >>>> Cheers,
> >>>>   Albert
> >>>>
> >>>>>
> >>>>> Best regards,
> >>>>> Adam
> >>>>>
> >>>>> Am 07.05.2018 um 19:14 schrieb Albert Astals Cid:
> >>>>>>  poppler/Array.cc |    2 +-
> >>>>>>  poppler/Dict.cc  |    2 +-
> >>>>>>  2 files changed, 2 insertions(+), 2 deletions(-)
> >>>>>>
> >>>>>> New commits:
> >>>>>> commit 07b8f838b3d8859a3ad34a3140bb24475bd6ac2c
> >>>>>> Author: Albert Astals Cid <aacid at kde.org>
> >>>>>> Date:   Mon May 7 19:13:07 2018 +0200
> >>>>>>
> >>>>>>     cast to void * to bypass new gcc -Wclass-memaccess warning
> >>>>>>     
> >>>>>>     Yes what we're doing there is a bit nasty but it works :D
> >>>>>>
> >>>>>> diff --git a/poppler/Array.cc b/poppler/Array.cc
> >>>>>> index 3276349f..e8aa34ea 100644
> >>>>>> --- a/poppler/Array.cc
> >>>>>> +++ b/poppler/Array.cc
> >>>>>> @@ -112,7 +112,7 @@ void Array::remove(int i) {
> >>>>>>
> >>>>>>  #endif
> >>>>>>  
> >>>>>>    }
> >>>>>>    --length;
> >>>>>>
> >>>>>> -  memmove( elems + i, elems + i + 1, sizeof(elems[0]) * (length - i) );
> >>>>>> +  memmove( static_cast<void*>(elems + i), elems + i + 1, sizeof(elems[0])
> >>>>>> * (length - i) );> 
> >>>>>>  }
> >>>>>>  
> >>>>>>  Object Array::get(int i, int recursion) const {
> >>>>>>
> >>>>>> diff --git a/poppler/Dict.cc b/poppler/Dict.cc
> >>>>>> index a431f7eb..bc86fd77 100644
> >>>>>> --- a/poppler/Dict.cc
> >>>>>> +++ b/poppler/Dict.cc
> >>>>>> @@ -201,7 +201,7 @@ void Dict::remove(const char *key) {
> >>>>>>
> >>>>>>        gfree(entries[pos].key);
> >>>>>>        entries[pos].val.free();
> >>>>>>        if (pos != length) {
> >>>>>>
> >>>>>> -        memmove(&entries[pos], &entries[pos + 1], (length - pos) *
> >>>>>> sizeof(DictEntry)); +        memmove(static_cast<void*>(&entries[pos]),
> >>>>>> &entries[pos + 1], (length - pos) * sizeof(DictEntry));> 
> >>>>>>        }
> >>>>>>      
> >>>>>>      }
> >>>>>>    
> >>>>>>    } else {
> >>>>>>
> >>>>>> _______________________________________________
> >>>>>> poppler mailing list
> >>>>>> poppler at lists.freedesktop.org
> >>>>>> https://lists.freedesktop.org/mailman/listinfo/poppler
> >>>>
> >>>>
> >>>>
> >>>>
> >>>> _______________________________________________
> >>>> poppler mailing list
> >>>> poppler at lists.freedesktop.org
> >>>> https://lists.freedesktop.org/mailman/listinfo/poppler
> >>>>
> >>>
> >>>
> >>>
> >>> _______________________________________________
> >>> poppler mailing list
> >>> poppler at lists.freedesktop.org
> >>> https://lists.freedesktop.org/mailman/listinfo/poppler
> >>>
> >>>
> >>>
> >>> _______________________________________________
> >>> poppler mailing list
> >>> poppler at lists.freedesktop.org
> >>> https://lists.freedesktop.org/mailman/listinfo/poppler
> >>>
> >>>
> >>> _______________________________________________
> >>> poppler mailing list
> >>> poppler at lists.freedesktop.org
> >>> https://lists.freedesktop.org/mailman/listinfo/poppler