[poppler] Warnings in the glib/cairo sides

[suzuki toshiya]

Hi,

I think glib/poppler-attachment.cc::poppler_attachment_save should return `result'.

    168 gboolean
    169 poppler_attachment_save (PopplerAttachment  *attachment,
    170                          const char         *filename,
    171                          GError            **error)
    172 {
    173   gboolean result;
    174   FILE *f;
    175
    176   g_return_val_if_fail (POPPLER_IS_ATTACHMENT (attachment), FALSE);
    177
    178   f = g_fopen (filename, "wb");
    179
    180   if (f == NULL)
    181     {
    182       gchar *display_name = g_filename_display_name (filename);
    183       g_set_error (error,
    184                    G_FILE_ERROR,
    185                    g_file_error_from_errno (errno),
    186                    _("Failed to open '%s' for writing: %s"),
    187                    display_name,
    188                    g_strerror (errno));
    189       g_free (display_name);
    190       return FALSE;
    191     }
    192
    193   result = poppler_attachment_save_to_callback (attachment, save_helper, f, error);

poppler_attachment_save_to_callback() can return an error,
when save_helper() got an error in it.

    194
    195   if (fclose (f) < 0)
    196     {
    197       gchar *display_name = g_filename_display_name (filename);
    198       g_set_error (error,
    199                    G_FILE_ERROR,
    200                    g_file_error_from_errno (errno),
    201                    _("Failed to close '%s', all data may not have been saved: %s"),
    202                    display_name,
    203                    g_strerror (errno));
    204       g_free (display_name);
    205       return FALSE;
    206     }
    207
    208   return TRUE;
    209 }

After calling save_helper(), current implementation only cares whether the file
stream can be closed correctly and ignores whether save_helper() finishes successfully.
There might be a question if we should distinguish "save_helper() is ok but closing
the stream is not ok" versus "save_helper() is not ok", but, at least, the last
line should be "return result", not "return TRUE".

Regards,
mpsuzuki

diff --git a/glib/poppler-attachment.cc b/glib/poppler-attachment.cc
index 1218b9b..06a3aac 100644
--- a/glib/poppler-attachment.cc
+++ b/glib/poppler-attachment.cc
@@ -205,7 +205,7 @@ poppler_attachment_save (PopplerAttachment  *attachment,
       return FALSE;
     }

-  return TRUE;
+  return result;
 }

 #define BUF_SIZE 1024



Albert Astals Cid wrote:
> gcc 4.6 gives
> 
> glib/poppler-attachment.cc:173:12:
> 	warning: variable ‘result’ set but not used [-Wunused-but-set-variable]
> 
> poppler/CairoFontEngine.cc:109:17:
> 	warning: variable ‘w3’ set but not used [-Wunused-but-set-variable]
> 
> poppler/CairoRescaleBox.cc:263:11:
> 	warning: variable ‘retval’ set but not used [-Wunused-but-set-variable]
> 
> Can you cairo/glib guys have a look at if we should be using them or if we can 
> just kill them?
> 
> Thanks,
>   Albert
> _______________________________________________
> poppler mailing list
> poppler at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/poppler