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

[Adam Reichold]

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.

> Cheers,
>   Albert

Best regards,
Adam

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/poppler/attachments/20180416/51857ed7/attachment-0001.sig>