[poppler] poppler Digest, Vol 64, Issue 9

[Pino Toscano]

Hi,

patches gets better, although there are still some things that can be 
improved:

> +  // Return the PDF ID contains in the trailer dictionary 
> +  char** getID();

I still think we could just use inout parameters for the two strings, ie 
something like:

  void PDFDoc::getId(char id1[33], char id2[33])

  char id1[33];
  char id2[33];
  doc->getId(id1, id2);

what do the others think about this?

> +static void
> +get_id(char* encodedid,char *pdfid) {
> +
> +       char buf[3];
> +       for(int i=0;i<16;i++) {
> +
> +               sprintf(buf,"%02x",encodedid[i] & 0xff);
> +
> +               if(i == 0) strcpy(pdfid,buf);
> +               else    strcat(pdfid,buf);
> +       }
> +}

Thinking further, this can be simplified a lot, eg:

  sprintf(pdfid, "%02x%02x%02x...",
    encodedid[0] & 0xff,
    encodedid[1] & 0xff,
    encodedid[2] & 0xff,
  [...]
  );

Yes, a bit longer, but much faster.

> +GBool PDFDoc::setID() {
> +
> +       char **pdfid = getID();
> +       if(pdfid != NULL) return false;

if pdfid != NULL then you are leaking it.

> +       else {
> +               char *filename = getFileName()->getCString(); 

You are relying on the fact that a PDFDoc was opened from a local file, 
so fileName (hence getFileName()) returns a non-NULL GooString.
This is not always the case, so you must handle that situation.

> +
> +               GooString message;
> +               char buffer[256];

64 chars are more than enough...

> +               // file size
> +               unsigned int fileSize = 0;
> +                       
> +               FILE *f;
> +               f = fopen(filename,"r");
> +               char ch;
> +               while((ch=fgetc(f)) != EOF) {
> +                       fileSize++;
> +               }

This is a bit slow... A better way would be seeking to the end and see 
the position, falling back to this manual reading if the seek fails.

> +               if (!(f = fopen(filename,"r+"))) {
> +                       error(-1, "Couldn't open file
> '%s'",filename);
> +               }

Same as above, this PDFDoc can also be constructed in other ways than 
from a physical file.

>        break;
> +    case PROP_PERMANENT_ID:
> +       char **pid;
> +       pid = document->doc->getID();
> +       if(pid != NULL) {
> +               g_value_set_string(value,strdup(pid[0]));
> +               free(pid);

The current getID() returns a pointer to an array of two char pointers. 
This means the free() will just delete the array of char*, but not the 
two actual char*.
(Similar problem in other places.)

> +                               "Permanent Id of the Pdf Document",
> [...]
> +                               "Update Id of the Pdf Document",

"Pdf" -> "PDF"

> +
> +PopplerDocumentId*
> +poppler_document_get_pdf_id (PopplerDocument *document)
> +{
> +       char **pid = document->doc->getID();
> +       
> +       if(pid == NULL) {
> +               free(pid);
> +               return NULL;
> +       }
> +       else 
> +       {
> +               PopplerDocumentId *doc_id =
> g_new(PopplerDocumentId,1);
> +               strcpy(doc_id->permanent_id,strdup(pid[0]));
> +               strcpy(doc_id->update_id,strdup(pid[1]));

pid[0] and pid[1] are the brand-new strings getID() returns, so you are 
duplicating them and then copying the new copies on the 
PopplerDocumentId fields; this mean the two intermediate strdup() are 
leaked.

> +gboolean
> +poppler_document_set_pdf_id (PopplerDocument *document) 
> +{
> +       GBool boolean = document->doc->setID();
> +       if(boolean) return TRUE;
> +       else return FALSE;
> +}

Easier:
  return document->doc->setID() ? TRUE : FALSE;
(this is subject to Carlos' preference though)

-- 
Pino Toscano
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 190 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.freedesktop.org/archives/poppler/attachments/20100615/eeba0813/attachment.pgp>