[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 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?

> 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.

Cheers,
  Albert

> 
> To keep the original semantics one must copy if I understand things
> correctly.