[PATCH] Remove warning 'format not a string literal' in libsvn_client

[Daniel Näslund]

Hi!

Still on the lookout for something interresting. In the meantime I've
started removing some compiler warnings.

The code looks uglier but the warnings is gone. Is there any #directive
to ignore warnings for a subpart of the code? We know that those fmt
strings are safe. Perhaps we could tell the compiler that?

[[[
* subversion/libsvn_client/diff.c
  (display_prop_diffs): Remove warnings about 'format not a string
    literal'.

* subversion/libsvn_client/export.c
  (copy_one_versioned_file): Remove warnings about 'format not a string
    literal'.

Patch by: Daniel Näslund <daniel@...>
]]]

/Daniel

[format_warning.diff]

---

Index: subversion/libsvn_client/export.c
===================================================================
--- subversion/libsvn_client/export.c (revision 40378)
+++ subversion/libsvn_client/export.c (arbetskopia)
@@ -204,7 +204,6 @@
   if (keywords)
     {
       svn_revnum_t changed_rev;
-      const char *fmt;
       const char *author;
 
       SVN_ERR(svn_wc__node_get_changed_info(&changed_rev, NULL, &author,
@@ -217,18 +216,19 @@
              to the revision number, and set the author to
              "(local)" since we can't always determine the
              current user's username */
-          fmt = "%ldM";
           author = _("(local)");
+          SVN_ERR(svn_subst_build_keywords2
+                  (&kw, keywords->data,
+                   apr_psprintf(scratch_pool, "%ldM", changed_rev),
+                   entry->url, tm, author, scratch_pool));
         }
       else
         {
-          fmt = "%ld";
+          SVN_ERR(svn_subst_build_keywords2
+                  (&kw, keywords->data,
+                   apr_psprintf(scratch_pool, "%ld", changed_rev),
+                   entry->url, tm, author, scratch_pool));
         }
-
-      SVN_ERR(svn_subst_build_keywords2
-              (&kw, keywords->data,
-               apr_psprintf(scratch_pool, fmt, changed_rev),
-               entry->url, tm, author, scratch_pool));
     }
 
   /* For atomicity, we translate to a tmp file and then rename the tmp file
Index: subversion/libsvn_client/diff.c
===================================================================
--- subversion/libsvn_client/diff.c (revision 40378)
+++ subversion/libsvn_client/diff.c (arbetskopia)
@@ -229,7 +229,6 @@
 
   for (i = 0; i < propchanges->nelts; i++)
     {
-      const char *header_fmt;
       const svn_string_t *original_value;
       const svn_prop_t *propchange =
         &APR_ARRAY_IDX(propchanges, i, svn_prop_t);
@@ -248,12 +247,13 @@
         continue;
 
       if (! original_value)
-        header_fmt = _("Added: %s%s");
+        SVN_ERR(file_printf_from_utf8(file, encoding, "Added: %s%s",
+                                    propchange->name, APR_EOL_STR));
       else if (! propchange->value)
-        header_fmt = _("Deleted: %s%s");
+        SVN_ERR(file_printf_from_utf8(file, encoding, "Deleted: %s%s",
+                                    propchange->name, APR_EOL_STR));
       else
-        header_fmt = _("Modified: %s%s");
-      SVN_ERR(file_printf_from_utf8(file, encoding, header_fmt,
+        SVN_ERR(file_printf_from_utf8(file, encoding, "Modified: %s%s",
                                     propchange->name, APR_EOL_STR));
 
       if (strcmp(propchange->name, SVN_PROP_MERGEINFO) == 0)

---

[Julian Foad]

Daniel Näslund wrote:
> Still on the lookout for something interresting. In the meantime I've
> started removing some compiler warnings.
>
> The code looks uglier but the warnings is gone.

A statement like that is a red flag to me. We are not slaves to the
warnings; we should only use them as a hint to try to write "better"
code.

>  Is there any #directive
> to ignore warnings for a subpart of the code? We know that those fmt
> strings are safe. Perhaps we could tell the compiler that?

That would be better than writing ugly code, but probably not necessary.

> [[[
> * subversion/libsvn_client/diff.c
>   (display_prop_diffs): Remove warnings about 'format not a string
>     literal'.
>
> * subversion/libsvn_client/export.c
>   (copy_one_versioned_file): Remove warnings about 'format not a string
>     literal'.
>
> Patch by: Daniel Näslund <daniel@...>
> ]]]

For this bit:

>        if (! original_value)
> -        header_fmt = _("Added: %s%s");
> +        SVN_ERR(file_printf_from_utf8(file, encoding, "Added: %s%s",
> +                                    propchange->name, APR_EOL_STR));
>        else if (! propchange->value)
> -        header_fmt = _("Deleted: %s%s");
> +        SVN_ERR(file_printf_from_utf8(file, encoding, "Deleted: %s%s",
> +                                    propchange->name, APR_EOL_STR));
>        else
> -        header_fmt = _("Modified: %s%s");
> -      SVN_ERR(file_printf_from_utf8(file, encoding, header_fmt,
> +        SVN_ERR(file_printf_from_utf8(file, encoding, "Modified: %s%s",
>                                      propchange->name, APR_EOL_STR));

consider writing:

       if (! original_value)
         action = _("Added");
       else if (! propchange->value)
         action = _("Deleted");
       else
         action = _("Modified");
       SVN_ERR(file_printf_from_utf8(file, encoding,
                                     "%s: %s" APR_EOL_STR,
                                     action, propchange->name));

and similarly, in the other bit, you could do:

[... suffix = "M" or "" ...]

       SVN_ERR(svn_subst_build_keywords2
               (&kw, keywords->data,
-               apr_psprintf(scratch_pool, fmt, changed_rev),
+               apr_psprintf(scratch_pool, "%ld%s",
+                            changed_rev, suffix),
                entry->url, tm, author, scratch_pool));

- Julian

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2414666

[Peter Samuelson]

[Daniel Näslund]
>        if (! original_value)
> -        header_fmt = _("Added: %s%s");
> +        SVN_ERR(file_printf_from_utf8(file, encoding, "Added: %s%s",
> +                                    propchange->name, APR_EOL_STR));

Julian talked about this code already, but just to point out: your
change is incorrect, because it removes the _() functionality, i.e.,
string localisation.  You have to be careful about these things.

...Indeed, we use quite a lot of _() with printf format strings (see
all the references to 'c-format' in tools/po/po-update.sh).  Why
aren't these _all_ warnings with your compiler?
--
Peter Samuelson | org-tld!p12n!peter | http://p12n.org/

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2414768

[Julian Foad]

Peter Samuelson wrote:
> ...Indeed, we use quite a lot of _() with printf format strings (see
> all the references to 'c-format' in tools/po/po-update.sh).  Why
> aren't these _all_ warnings with your compiler?

Peter,

The warnings are nothing to do with _() but only to do with passing a
variable as the format argument to a printf(), which makes the compiler
unable to check the format.

- Julian

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2414816

[Stefan Sperling-7]

On Thu, Nov 05, 2009 at 05:37:46PM +0000, Julian Foad wrote:

> Peter Samuelson wrote:
> > ...Indeed, we use quite a lot of _() with printf format strings (see
> > all the references to 'c-format' in tools/po/po-update.sh).  Why
> > aren't these _all_ warnings with your compiler?
>
> Peter,
>
> The warnings are nothing to do with _() but only to do with passing a
> variable as the format argument to a printf(), which makes the compiler
> unable to check the format.

And passing a variable as a format string is always wrong, isn't it?

Stefan

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2414832

[Daniel Näslund]

On Thu, Nov 05, 2009 at 11:19:37AM +0000, Julian Foad wrote:
[[[
* subversion/libsvn_client/diff.c
  (display_prop_diffs): Remove warnings about 'format not a string
    literal'.

* subversion/libsvn_client/export.c
  (copy_one_versioned_file): Remove warnings about 'format not a string
    literal'.

Patch by: Daniel Näslund <daniel@...>
          Julian Foad
]]]

> For this bit:
>
> >        if (! original_value)
> > -        header_fmt = _("Added: %s%s");
> > +        SVN_ERR(file_printf_from_utf8(file, encoding, "Added: %s%s",
> > +                                    propchange->name, APR_EOL_STR));
> >        else if (! propchange->value)
> > -        header_fmt = _("Deleted: %s%s");
> > +        SVN_ERR(file_printf_from_utf8(file, encoding, "Deleted: %s%s",
> > +                                    propchange->name, APR_EOL_STR));
> >        else
> > -        header_fmt = _("Modified: %s%s");
> > -      SVN_ERR(file_printf_from_utf8(file, encoding, header_fmt,
> > +        SVN_ERR(file_printf_from_utf8(file, encoding, "Modified: %s%s",
> >                                      propchange->name, APR_EOL_STR));
>
> consider writing:
>
>        if (! original_value)
>          action = _("Added");
>        else if (! propchange->value)
>          action = _("Deleted");
>        else
>          action = _("Modified");
>        SVN_ERR(file_printf_from_utf8(file, encoding,
>                                      "%s: %s" APR_EOL_STR,
>                                      action, propchange->name));
>
> and similarly, in the other bit, you could do:
>
> [... suffix = "M" or "" ...]
>
>        SVN_ERR(svn_subst_build_keywords2
>                (&kw, keywords->data,
> -               apr_psprintf(scratch_pool, fmt, changed_rev),
> +               apr_psprintf(scratch_pool, "%ld%s",
> +                            changed_rev, suffix),
>                 entry->url, tm, author, scratch_pool));

That was so much better. Very few lines in the previous patch but I
managed to screw up a couple of localisation strings and do some heavy
code duplication. Not bad.

I've run the export_tests and diff_tests. They pass.

/Daniel

[format_warnings.diff]

---

Index: subversion/libsvn_client/export.c
===================================================================
--- subversion/libsvn_client/export.c (revision 40386)
+++ subversion/libsvn_client/export.c (arbetskopia)
@@ -204,7 +204,7 @@
   if (keywords)
     {
       svn_revnum_t changed_rev;
-      const char *fmt;
+      const char *suffix;
       const char *author;
 
       SVN_ERR(svn_wc__node_get_changed_info(&changed_rev, NULL, &author,
@@ -217,17 +217,17 @@
              to the revision number, and set the author to
              "(local)" since we can't always determine the
              current user's username */
-          fmt = "%ldM";
+          suffix = "M";
           author = _("(local)");
         }
       else
         {
-          fmt = "%ld";
+          suffix = "";
         }
 
       SVN_ERR(svn_subst_build_keywords2
               (&kw, keywords->data,
-               apr_psprintf(scratch_pool, fmt, changed_rev),
+               apr_psprintf(scratch_pool, "%ld%s", changed_rev, suffix),
                entry->url, tm, author, scratch_pool));
     }
 
Index: subversion/libsvn_client/diff.c
===================================================================
--- subversion/libsvn_client/diff.c (revision 40386)
+++ subversion/libsvn_client/diff.c (arbetskopia)
@@ -229,7 +229,7 @@
 
   for (i = 0; i < propchanges->nelts; i++)
     {
-      const char *header_fmt;
+      const char *action;
       const svn_string_t *original_value;
       const svn_prop_t *propchange =
         &APR_ARRAY_IDX(propchanges, i, svn_prop_t);
@@ -248,12 +248,12 @@
         continue;
 
       if (! original_value)
-        header_fmt = _("Added: %s%s");
+        action = _("Added");
       else if (! propchange->value)
-        header_fmt = _("Deleted: %s%s");
+        action = _("Deleted");
       else
-        header_fmt = _("Modified: %s%s");
-      SVN_ERR(file_printf_from_utf8(file, encoding, header_fmt,
+        action = _("Modified");
+      SVN_ERR(file_printf_from_utf8(file, encoding, "%s: %s%s", action,
                                     propchange->name, APR_EOL_STR));
 
       if (strcmp(propchange->name, SVN_PROP_MERGEINFO) == 0)

---

[Branko Cibej]

Peter Samuelson wrote:

> [Daniel Näslund]
>  
>>        if (! original_value)
>> -        header_fmt = _("Added: %s%s");
>> +        SVN_ERR(file_printf_from_utf8(file, encoding, "Added: %s%s",
>> +                                    propchange->name, APR_EOL_STR));
>>    
>
> Julian talked about this code already, but just to point out: your
> change is incorrect, because it removes the _() functionality, i.e.,
> string localisation.  You have to be careful about these things.
>
> ...Indeed, we use quite a lot of _() with printf format strings (see
> all the references to 'c-format' in tools/po/po-update.sh).  Why
> aren't these _all_ warnings with your compiler?
>  

Because his compiler is probably gcc, and gcc "sees" through gettext()
and friends; in other words, libintl.h declares the necessary function
attributes that allow gcc to make assumptions about the return values
based on function arguments.

-- Brane

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2414863

[Daniel Näslund]

On Thu, Nov 05, 2009 at 07:18:29PM +0100, Branko Cibej wrote:

> Peter Samuelson wrote:
> > [Daniel Näslund]
> >  
> >>        if (! original_value)
> >> -        header_fmt = _("Added: %s%s");
> >> +        SVN_ERR(file_printf_from_utf8(file, encoding, "Added: %s%s",
> >> +                                    propchange->name, APR_EOL_STR));
> >>    
> >
> > Julian talked about this code already, but just to point out: your
> > change is incorrect, because it removes the _() functionality, i.e.,
> > string localisation.  You have to be careful about these things.
> >
> > ...Indeed, we use quite a lot of _() with printf format strings (see
> > all the references to 'c-format' in tools/po/po-update.sh).  Why
> > aren't these _all_ warnings with your compiler?
> >  
>
> Because his compiler is probably gcc, and gcc "sees" through gettext()
> and friends; in other words, libintl.h declares the necessary function
> attributes that allow gcc to make assumptions about the return values
> based on function arguments.

In my libintl.h gettext is declared like this:

  extern char *gettext (__const char *__msgid)
       __THROW __attribute_format_arg__ (1);

Where __attribute_format_arg__ specifies that the function takes a
format string and modifies it but preservers the rest of the %s parameters. [1]

But I still haven't found out how _("...") is expanded to
gettext("...").

Thanks Branko for the information. This day wasn't wasted. I've learned
something new.

/Daniel

[1] http://gcc.gnu.org/onlinedocs/gcc/Function-Attributes.html

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2414876

[Daniel Shahaf-2]

Daniel Näslund wrote on Thu, 5 Nov 2009 at 19:57 +0100:
> But I still haven't found out how _("...") is expanded to
> gettext("...").
>

It's defined in svn_private_config.h.

> Thanks Branko for the information. This day wasn't wasted. I've learned
> something new.

Same.

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2414882

[Julian Foad]

Daniel Näslund wrote:

> On Thu, Nov 05, 2009 at 11:19:37AM +0000, Julian Foad wrote:
> [[[
> * subversion/libsvn_client/diff.c
>   (display_prop_diffs): Remove warnings about 'format not a string
>     literal'.
>
> * subversion/libsvn_client/export.c
>   (copy_one_versioned_file): Remove warnings about 'format not a string
>     literal'.
>
> Patch by: Daniel Näslund <daniel@...>
>           Julian Foad
> ]]]

[...]
> That was so much better. Very few lines in the previous patch but I
> managed to screw up a couple of localisation strings and do some heavy
> code duplication. Not bad.
>
> I've run the export_tests and diff_tests. They pass.

Thanks! Committed revision 40389.

I tried to write the log message in a way that doesn't imply that
removing warnings is the "be-all and end-all" of why we want this
change. In other words, the warnings gave us a hint that we might like
to make an improvement to our code, but we made the change because it is
an improvement, not just because it stops the warnings from appearing.

- Julian

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2414883

[Branko Cibej]

Daniel Näslund wrote:
> But I still haven't found out how _("...") is expanded to
> gettext("...").
>  

It's defined in svn_private_config.h.

> Thanks Branko for the information. This day wasn't wasted. I've learned
> something new.
>  

:)

-- Brane

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2414922

[Gavin Baumanis-4]

Hi Daniel,

Just checking to see if you're doing any more work with this patch  
submission?

Gavin.


On 06/11/2009, at 05:57 , Daniel Näslund wrote:

> On Thu, Nov 05, 2009 at 07:18:29PM +0100, Branko Cibej wrote:
>> Peter Samuelson wrote:
>>> [Daniel Näslund]
>>>
>>>>       if (! original_value)
>>>> -        header_fmt = _("Added: %s%s");
>>>> +        SVN_ERR(file_printf_from_utf8(file, encoding, "Added: %s
>>>> %s",
>>>> +                                    propchange->name,  
>>>> APR_EOL_STR));
>>>>
>>>
>>> Julian talked about this code already, but just to point out: your
>>> change is incorrect, because it removes the _() functionality, i.e.,
>>> string localisation.  You have to be careful about these things.
>>>
>>> ...Indeed, we use quite a lot of _() with printf format strings (see
>>> all the references to 'c-format' in tools/po/po-update.sh).  Why
>>> aren't these _all_ warnings with your compiler?
>>>
>>
>> Because his compiler is probably gcc, and gcc "sees" through gettext
>> ()
>> and friends; in other words, libintl.h declares the necessary  
>> function
>> attributes that allow gcc to make assumptions about the return values
>> based on function arguments.
>
> In my libintl.h gettext is declared like this:
>
>  extern char *gettext (__const char *__msgid)
>       __THROW __attribute_format_arg__ (1);
>
> Where __attribute_format_arg__ specifies that the function takes a
> format string and modifies it but preservers the rest of the %s  
> parameters. [1]
>
> But I still haven't found out how _("...") is expanded to
> gettext("...").
>
> Thanks Branko for the information. This day wasn't wasted. I've  
> learned
> something new.
>
> /Daniel
>
> [1] http://gcc.gnu.org/onlinedocs/gcc/Function-Attributes.html
>
> ------------------------------------------------------
> http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2414876

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2416310

[Daniel Näslund]

Hi Gavin!

This one has already been submitted. r40389

On Wed, Nov 11, 2009 at 10:11:45AM +1100, Gavin 'Beau' Baumanis wrote:

> Hi Daniel,
>
> Just checking to see if you're doing any more work with this patch  
> submission?
>
> Gavin.
>
>
> On 06/11/2009, at 05:57 , Daniel Näslund wrote:
>
>> On Thu, Nov 05, 2009 at 07:18:29PM +0100, Branko Cibej wrote:
>>> Peter Samuelson wrote:
>>>> [Daniel Näslund]
>>>>
>>>>>       if (! original_value)
>>>>> -        header_fmt = _("Added: %s%s");
>>>>> +        SVN_ERR(file_printf_from_utf8(file, encoding, "Added: %s
>>>>> %s",
>>>>> +                                    propchange->name,  
>>>>> APR_EOL_STR));
>>>>>
>>>>
>>>> Julian talked about this code already, but just to point out: your
>>>> change is incorrect, because it removes the _() functionality, i.e.,
>>>> string localisation.  You have to be careful about these things.
>>>>
>>>> ...Indeed, we use quite a lot of _() with printf format strings (see
>>>> all the references to 'c-format' in tools/po/po-update.sh).  Why
>>>> aren't these _all_ warnings with your compiler?

[...]

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2416403

[Gavin Baumanis-4]

Great news - thanks Daniel.

Gavin.


On 11/11/2009, at 17:34 , Daniel Näslund wrote:

> Hi Gavin!
>
> This one has already been submitted. r40389
>
> On Wed, Nov 11, 2009 at 10:11:45AM +1100, Gavin 'Beau' Baumanis wrote:
>> Hi Daniel,
>>
>> Just checking to see if you're doing any more work with this patch  
>> submission?
>>
>> Gavin.
>>
>>
>> On 06/11/2009, at 05:57 , Daniel Näslund wrote:
>>
>>> On Thu, Nov 05, 2009 at 07:18:29PM +0100, Branko Cibej wrote:
>>>> Peter Samuelson wrote:
>>>>> [Daniel Näslund]
>>>>>
>>>>>>      if (! original_value)
>>>>>> -        header_fmt = _("Added: %s%s");
>>>>>> +        SVN_ERR(file_printf_from_utf8(file, encoding, "Added: %s
>>>>>> %s",
>>>>>> +                                    propchange->name,  
>>>>>> APR_EOL_STR));
>>>>>>
>>>>>
>>>>> Julian talked about this code already, but just to point out: your
>>>>> change is incorrect, because it removes the _() functionality, i.e.,
>>>>> string localisation.  You have to be careful about these things.
>>>>>
>>>>> ...Indeed, we use quite a lot of _() with printf format strings (see
>>>>> all the references to 'c-format' in tools/po/po-update.sh).  Why
>>>>> aren't these _all_ warnings with your compiler?
>
> [...]
>
> ------------------------------------------------------
> http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2416403

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2416420