<html> <head> <base href="https://bugs.freedesktop.org/" /> </head> <body> <div> <a class="bz_bug_link bz_status_NEW " title="NEW --- - Add new PDFWriter class for writing PDF with printing options" href="https://bugs.freedesktop.org/show_bug.cgi?id=71213#c11">Comment # 11</a> on <a class="bz_bug_link bz_status_NEW " title="NEW --- - Add new PDFWriter class for writing PDF with printing options" href="https://bugs.freedesktop.org/show_bug.cgi?id=71213">bug 71213</a> from <a class="email" href="mailto:ajohnson@redneon.com" title="Adrian Johnson <ajohnson@redneon.com>"> Adrian Johnson</a> <pre>Created <a href="attachment.cgi?id=89020" name="attach_89020" title="PDFWriter changes to address review comments">attachment 89020</a> <a href="attachment.cgi?id=89020&action=edit" title="PDFWriter changes to address review comments">[details]</a> <a href='page.cgi?id=splinter.html&bug=71213&attachment=89020'>[review]</a> PDFWriter changes to address review comments I've addressed most of the review comments and have attached a patch with the changes. I'll wait for the other patches to be reviewed before rolling this patch back into the original PDFWriter patch so I only have to rebase all the patches once. The comments that I didn't fix are: 1) >@@ +186,5 @@ >> + mediaSize->y2 = paperSizes[0].width; >> + margins->x1 = paperSizes[0].bottom; >> + margins->x2 = mediaSize->y2 - paperSizes[0].top; >> + margins->y1 = paperSizes[0].left; >> + margins->y2 = mediaSize->x2 - paperSizes[0].right; > >This block is repeated a lot, I wonder if we could add a helper function, > something like setSizeAndMargins(PDFRectangle *mediaSize, PDFRectangle > *margins, double with, double height, double top, double bottom, double left, > double right); I could be static inline The problem I had with this is we will have a function with six parameters. When looking at a call to this function it will be hard to keep track which parameter does what. In the example quoted above the parameter list will end up being quite long so I don't see the value in turning it into a function. 2) signed vs unsigned > > + for (int i = 0; i < (int)paperSizes.size(); i++) { > Why not using unsigned for i instead of casting? I prefer to always use signed integers unless I am doing modular arithmetic or bit manipulation. Mixing signed and unsigned can produce unexpected results. So it is better to just use signed all the time. The cast adds an extra 5 characters but then so does replacing 'int' with 'unsigned'. >> + for (int pg = (int)pageRefs[0].size() - 1; pg >= 0; pg--) { > Ditto. If this is changed to unsigned the loop never exits.</pre> </div> <hr> You are receiving this mail because: <ul> <li>You are the assignee for the bug.</li> </ul> </body> </html>