[poppler] Can anyone have a look at my patch?

[Adam Reichold]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hello Adrian,

Am 31.10.2014 um 11:48 schrieb Adrian Johnson:
> On 31/10/14 18:49, Adam Reichold wrote: Hello Albert,
> 
> Am 31.10.2014 um 00:01 schrieb Albert Astals Cid:
>>>> Hi guys, i made a patch for PSOutputDev and i think it's 
>>>> good, but extra set of eyes are never a bad thing.
>>>> 
>>>> https://bugs.freedesktop.org/attachment.cgi?id=108708
>>>> 
>>>> Basically it's so that you don't have to pass 
>>>> firstPage-lastPage to PSOutputDev and can simply pass an 
>>>> arbitrary set of pages.
> 
> I had a look and I think it is correct. However, I did only test
> it using "pdftops" and not using the Glib or Qt code paths.
> 
> As a small style improvement, I would suggest turning "pgi" into a 
> "size_t" to get rid of the cast. I would also replace "pg" of type 
> "int" within "writeDocSetup" by a "pgi" of type "size_t" to remove
> the cast and any chance of possible confusion, i.e. someone 
> forgetting that "pg" is not the page number anymore, but an index 
> to the page vector.
> 
>> I disagree. All arithmetic (including counting) should be done 
>> with signed. Leave the unsigned ints for bit manipulation and 
>> modular arithmetic.

I would wholeheartedly agree with the safety argument if we were
actually counting things or otherwise computing indices, but we are just
iterating. So IMHO, if one feels the need to write that cast, one could
also write an explicit iteration like

for (std::vector< int >::iterator pgit = pages.begin(); pgit !=
pages.end(); ++pgit) {
   const int pg = *pgit;
   /* ... */
}

instead, which removes the indexing completely yielding even more safety
and efficiency. But of course this is just so much stuff to write and
more importantly to read in an C++98 code base...

In any case, this is not something I would try to insist on and using
indices of type "int" is just as fine, as long as indices and pages are
separated.

Best regards, Adam.

> I just noticed this had already happened with the error message,
> so I'll attach a slightly modified version of your patch.
> 
> Best regards, Adam.
> 
>>>> Thanks, Albert
>>>> 
>>>> P.S: Yes i know i have lots of patches in the queue to review
>>>> P.S.2: I was planning to release 0.28 today but maybe 
>>>> tomorrow or over the weekend, i'd like to at least get 
>>>> https://bugs.freedesktop.org/show_bug.cgi?id=81724 and 
>>>> https://bugs.freedesktop.org/show_bug.cgi?id=85234 in.
>>>> 
>>>> _______________________________________________ poppler 
>>>> mailing list poppler at lists.freedesktop.org 
>>>> http://lists.freedesktop.org/mailman/listinfo/poppler
>>>> 
>> 
>> 
>> 
>> _______________________________________________ poppler mailing 
>> list poppler at lists.freedesktop.org 
>> http://lists.freedesktop.org/mailman/listinfo/poppler
>> 
> 
> 


-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)

iQEcBAEBAgAGBQJUU5IfAAoJEPSSjE3STU341m4H/1Q4rN3moLjTQmIKeqb38XYv
n1rSoaYIXcGxYJW5JMpk+aRyunRW6KqOtCSCrJ0cia6A4LmVNhtZkBFS33qPB7Qs
9A2KaJc0x4p1NKPaJdG2X/LXYI58M7mGsvwcmZMFIDGNCXpM2VfZww7OS9Xbn5g1
ydsxbPdoZFXDdf2DFQ2N8LpZsw29OTHsgE+GaDCJ4eLVHTm6k6wnBKLlnl0NG8qg
oiDU4FwyKM6fivC3BPhRB0Fp94DXzWXBeOKMBWGfxQPouk3sdrAigv7cEaNDPJrP
KiFv3DikW3lb5WG0fjIX5ie1oGy44nZKbKG31D0R54pwzkHTlKGpsZvLk36CjlM=
=F6gd
-----END PGP SIGNATURE-----