svn commit: r222084 - head/contrib/gperf/src
Bruce Evans
brde at optusnet.com.au
Tue May 31 09:14:01 UTC 2011
On Wed, 18 May 2011 mdf at FreeBSD.org wrote:
> On Wed, May 18, 2011 at 2:31 PM, Dimitry Andric <dim at freebsd.org> wrote:
>> On 2011-05-18 23:16, Pawel Jakub Dawidek wrote:
>>>
>>> On Wed, May 18, 2011 at 09:06:20PM +0000, Ben Laurie wrote:
>>>>
>>>> Author: benl
>>>> Date: Wed May 18 21:06:20 2011
>>>> New Revision: 222084
>>>> URL: http://svn.freebsd.org/changeset/base/222084
>>>>
>>>> Log:
>>>> Fix clang warnings.
>>>>
>>>> Approved by: philip (mentor)
>>>
>>> [...]
>>>>
>>>> - fprintf (stderr, " by changing asso_value['%c'] (char #%d)
>>>> to %d\n",
>>>> + fprintf (stderr, " by changing asso_value['%c'] (char #%zd)
>>>> to %d\n",
>>>> *p, p - union_set + 1, asso_values[(unsigned
>>>> char)(*p)]);
>>>
>>> Hmm, both 'p' and 'union_set' are 'char *' and %zd is for ssize_t. It is
>>> a bit strange that it fixes the warning.
>>
>> If you subtract two pointers, such as in this case, you get a ptrdiff_t.
>>
>> Strictly, this doesn't have to be exactly the same type as ssize_t, but
>> in practice it will almost always be.
>>
>> You can also cast the result to intmax_t, and use %jd, then it will
>> always be correct, but possibly have some small overhead.
>
> Or you can use %td which is the C99 conversion specifier for ptrdiff_t.
Of course this is the only correct fix.
All the changes are wrong IMO. Apart from being unmaintainable since they
are in dusty contrib code:
% Modified: head/contrib/gperf/src/gen-perf.cc
% ==============================================================================
% --- head/contrib/gperf/src/gen-perf.cc Wed May 18 21:04:29 2011 (r222083)
% +++ head/contrib/gperf/src/gen-perf.cc Wed May 18 21:06:20 2011 (r222084)
% @@ -246,7 +246,7 @@ Gen_Perf::change (List_Node *prior, List
% {
% if (option[DEBUG])
% {
% - fprintf (stderr, " by changing asso_value['%c'] (char #%d) to %d\n",
% + fprintf (stderr, " by changing asso_value['%c'] (char #%zd) to %d\n",
% *p, p - union_set + 1, asso_values[(unsigned char)(*p)]);
% fflush (stderr);
% }
%
%td
% Modified: head/contrib/gperf/src/key-list.cc
% ==============================================================================
% --- head/contrib/gperf/src/key-list.cc Wed May 18 21:04:29 2011 (r222083)
% +++ head/contrib/gperf/src/key-list.cc Wed May 18 21:06:20 2011 (r222084)
% @@ -497,8 +497,8 @@ Key_List::merge (List_Node *list1, List_
% *resultp = list1;
% break;
% }
% - if (occurrence_sort && list1->occurrence < list2->occurrence
% - || hash_sort && list1->hash_value > list2->hash_value)
% + if ((occurrence_sort && list1->occurrence < list2->occurrence)
% + || (hash_sort && list1->hash_value > list2->hash_value))
% {
% *resultp = list2;
% resultp = &list2->next; list2 = list1; list1 = *resultp;
It is a compiler bug to warn about precedence when there is no problem
with precedence, as here for && vs ||. clang recently became even
more broken than gcc for this -- it now warns even without -Wparentheses
(or -Wall, which implies -Wparentheses) in CFLAGS, so it issues broken
warning at very low WARNS levels (for WARNS=1, maybe even with no
WARNS).
% @@ -1035,17 +1035,16 @@ Key_List::output_hash_function (void)
% if (option[CPLUSPLUS])
% printf ("%s::", option.get_class_name ());
% printf ("%s ", option.get_hash_name ());
% - printf (option[KRC] ?
% - "(str, len)\n"
% - " register char *str;\n"
% - " register unsigned int len;\n" :
% - option[C] ?
% - "(str, len)\n"
% - " register const char *str;\n"
% - " register unsigned int len;\n" :
% - option[ANSIC] | option[CPLUSPLUS] ?
% - "(register const char *str, register unsigned int len)\n" :
% - "");
% + if (option[KRC] || option[C] || option [ANSIC] || option[CPLUSPLUS])
% + printf (option[KRC] ?
% + "(str, len)\n"
% + " register char *str;\n"
% + " register unsigned int len;\n" :
% + option[C] ?
% + "(str, len)\n"
% + " register const char *str;\n"
% + " register unsigned int len;\n" :
% + "(register const char *str, register unsigned int len)\n");
%
% /* Note that when the hash function is called, it has already been verified
% that min_key_len <= len <= max_key_len. */
Far too invasive, and I can't even see a problem in the original. The
original has an empty format for the default case. This is perfectly valid,
an serves as documentation for the default case. The expression is a
"computed switch" statement. The change also obfuscates the pseudo-cases
option[ANSIC] and option[CPLUSPLUS] as the default pseudo-case (after
filtering out the actual default case before the pseudo-switch.
% @@ -1442,7 +1441,7 @@ Key_List::output_lookup_array (void)
%
% if (option[DEBUG])
% fprintf (stderr,
% - "dup_ptr[%d]: hash_value = %d, index = %d, count = %d\n",
% + "dup_ptr[%zd]: hash_value = %d, index = %d, count = %d\n",
% dup_ptr - duplicates,
% dup_ptr->hash_value, dup_ptr->index, dup_ptr->count);
%
Looks like another ptrdiff_t.
% @@ -1986,17 +1985,16 @@ Key_List::output_lookup_function (void)
% if (option[CPLUSPLUS])
% printf ("%s::", option.get_class_name ());
% printf ("%s ", option.get_function_name ());
% - printf (option[KRC] ?
% - "(str, len)\n"
% - " register char *str;\n"
% - " register unsigned int len;\n" :
% - option[C] ?
% - "(str, len)\n"
% - " register const char *str;\n"
% - " register unsigned int len;\n" :
% - option[ANSIC] | option[CPLUSPLUS] ?
% - "(register const char *str, register unsigned int len)\n" :
% - "");
% + if (option[KRC] || option[C] || option[ANSIC] || option[CPLUSPLUS])
% + printf (option[KRC] ?
% + "(str, len)\n"
% + " register char *str;\n"
% + " register unsigned int len;\n" :
% + option[C] ?
% + "(str, len)\n"
% + " register const char *str;\n"
% + " register unsigned int len;\n" :
% + "(register const char *str, register unsigned int len)\n");
%
% /* Output the function's body. */
% printf ("{\n");
%
Another pseudo-switch invaded.
% Modified: head/contrib/gperf/src/options.cc
% ==============================================================================
% --- head/contrib/gperf/src/options.cc Wed May 18 21:04:29 2011 (r222083)
% +++ head/contrib/gperf/src/options.cc Wed May 18 21:06:20 2011 (r222084)
% @@ -237,7 +237,7 @@ Options::print_options (void)
% {
% putchar (*arg);
% arg++;
% - if (*arg >= 'A' && *arg <= 'Z' || *arg >= 'a' && *arg <= 'z')
% + if ((*arg >= 'A' && *arg <= 'Z') || (*arg >= 'a' && *arg <= 'z'))
% {
% putchar (*arg);
% arg++;
%
Another precedence non-problem.
Bruce
More information about the svn-src-head
mailing list