[poppler] 7 commits - CMakeLists.txt cpp/CMakeLists.txt poppler/GfxState.cc poppler/Link.cc poppler/Link.h qt5/src

[Albert Astals Cid]

El dilluns, 16 d’abril de 2018, a les 22:42:06 CEST, Adam Reichold va 
escriure:
> Am 16.04.2018 um 22:21 schrieb Albert Astals Cid:
> > El dilluns, 16 d’abril de 2018, a les 20:36:37 CEST, Adam Reichold va
> > 
> > escriure:
> >> Hello again,
> >> 
> >> Am 16.04.2018 um 20:22 schrieb Albert Astals Cid:
> >>> El dilluns, 16 d’abril de 2018, a les 19:05:25 CEST, Adam Reichold va
> >>> 
> >>> escriure:
> >>>> Hello,
> >>>> 
> >>>> concerning df8a4ee51e18a39f85568c4122e5edd8c03d61df, I think there is a
> >>>> problem in the "isArray" branch where a moved-from value
> >>>> "seenNextActions" is used if the array contains more than one dict. I
> >>>> think this is generally undefined behavior (even though it is probably
> >>>> fine in this case since a moved-from std::unique_ptr is probably just
> >>>> empty, but the only guarantee is that it can be destructed safely).
> >>>> 
> >>>> But I also think that it is not how the function is supposed to work
> >>>> since every call to parseActions should start we the same prefix of
> >>>> already-seen actions, hence needs to start with the same value of
> >>>> seenNextActions modifying its private copy if actual branching takes
> >>>> place, so that the copies probably cannot be elided in that part of the
> >>>> function.
> >>> 
> >>> You're right, good catch!
> >>> 
> >>>> Also if this is about performance, I think this could be a bit lazier
> >>>> by
> >>>> initializing the set the first when it will really be used, it could
> >>>> use
> >>>> std::unordered_set since we only check membership and a single lookup
> >>>> should suffice since it returns whether insertion actually took place.
> >>>> 
> >>>> But also since an empty instance of std::set does not allocate any
> >>>> nodes, wouldn't it be even simpler to just pass that set by value
> >>>> moving
> >>>> the set and not the pointer to it where that is possible? (It is
> >>>> basically the size of two pointers instead of one, but also looses one
> >>>> indirection.)
> >>> 
> >>> But your patch does exactly what i want to avoid, you're copying the set
> >>> in
> >>> 
> >>> auto seenNextActionsAux = seenNextActions;
> >>> 
> >>> This other patch I'm attaching should fix the problem too with less
> >>> copying, right?
> >> 
> >> It does not copy,
> > 
> > How does that not copy? it does
> > 
> >   setA = setB
> > 
> > that has to copy the contents, no?
> 
> I meant your second patch using std::shared_ptr. That one does not copy,
> but it also does not keep the semantics of the original implementation.
> My patch did copy where it is necessary to preserve those semantics.
> 
> >> but it is not functionally equivalent to the original
> >> implementation which would not share the already-seen actions between
> >> the branches in the array (as this does not imply circular connections).
> >> 
> >> Also std::shared_ptr uses atomic reference counts behind the scenes and
> >> is hence a rather heavy weight abstraction w.r.t performance.
> >> 
> >> So if any repetitions and not just circular paths are forbidden, the
> >> easiest option would probably be to have parseAction just become
> >> 
> >> static LinkAction* doParseAction(const Object*, const GooString*, const
> >> std::std<int>&);
> >> 
> >> with an additional helper
> >> 
> >> static LinkAction parseAction(const Object* obj, const GooString*
> >> baseURI) { std::set<int> seenNextActions;
> >> return doParseAction(obj, baseURI, seenNextActions);
> >> }
> > 
> > Yeah, that's what we have in some other places, i guess i'll just go with
> > that.
> 
> Concerning 444d9d8de5d4f8d627084405e1583b6d6d3900c7, I think it would be
> preferable to have second parseAction variant namend differently (and
> probably also private) so that it does not participate in overload
> resolution unintentionally and unnecessarily.

Made it private but did not rename it, having functions with optional 
parameters that then call the same function without optional parameters (or 
even extra ones) is a very common pattern.

Cheers,
  Albert

> 
> > Cheers,
> > 
> >   Albert
> 
> Best regards,
> Adam
> 
> >> To keep the original semantics one must copy if I understand things
> >> correctly.