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.
>
>
>
>
signature.asc
Description: OpenPGP digital signature
_______________________________________________ poppler mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/poppler
