Date: Tue, 31 May 2011 19:13:55 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: mdf@FreeBSD.org Cc: src-committers@FreeBSD.org, Pawel Jakub Dawidek <pjd@FreeBSD.org>, Ben Laurie <benl@FreeBSD.org>, svn-src-all@FreeBSD.org, Dimitry Andric <dim@FreeBSD.org>, svn-src-head@FreeBSD.org Subject: Re: svn commit: r222084 - head/contrib/gperf/src Message-ID: <20110531185629.J1721@besplex.bde.org> In-Reply-To: <BANLkTim=Vymb44bNyKbkW-oj=4kbsr0gxg@mail.gmail.com> References: <201105182106.p4IL6KkE008657@svn.freebsd.org> <20110518211651.GE2273@garage.freebsd.pl> <4DD43AB7.7060705@FreeBSD.org> <BANLkTim=Vymb44bNyKbkW-oj=4kbsr0gxg@mail.gmail.com>
index | next in thread | previous in thread | raw e-mail
On Wed, 18 May 2011 mdf@FreeBSD.org wrote: > On Wed, May 18, 2011 at 2:31 PM, Dimitry Andric <dim@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. Brucehelp
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20110531185629.J1721>