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>
next in thread | previous in thread | raw e-mail | index | archive | help
This message is in MIME format. The first part should be readable text, while the remaining parts are likely unreadable without MIME-aware tools. --0-1933995627-1306833235=:1721 Content-Type: TEXT/PLAIN; charset=X-UNKNOWN; format=flowed Content-Transfer-Encoding: QUOTED-PRINTABLE 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: >>>> =A0 Fix clang warnings. >>>> >>>> =A0 Approved by: philip (mentor) >>> >>> [...] >>>> >>>> - =A0 =A0 =A0 =A0 =A0 =A0fprintf (stderr, " by changing asso_value['%c= '] (char #%d) >>>> to %d\n", >>>> + =A0 =A0 =A0 =A0 =A0 =A0fprintf (stderr, " by changing asso_value['%c= '] (char #%zd) >>>> to %d\n", >>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 *p, p - union_set + 1, ass= o_values[(unsigned >>>> char)(*p)]); >>> >>> Hmm, both 'p' and 'union_set' are 'char *' and %zd is for ssize_t. It i= s >>> 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 % =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D % --- head/contrib/gperf/src/gen-perf.cc=09Wed May 18 21:04:29 2011=09(r222= 083) % +++ head/contrib/gperf/src/gen-perf.cc=09Wed May 18 21:06:20 2011=09(r222= 084) % @@ -246,7 +246,7 @@ Gen_Perf::change (List_Node *prior, List % { % if (option[DEBUG]) % { % - fprintf (stderr, " by changing asso_value['%c'] (char #%d) t= o %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 % =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D % --- head/contrib/gperf/src/key-list.cc=09Wed May 18 21:04:29 2011=09(r222= 083) % +++ head/contrib/gperf/src/key-list.cc=09Wed May 18 21:06:20 2011=09(r222= 084) % @@ -497,8 +497,8 @@ Key_List::merge (List_Node *list1, List_ % *resultp =3D list1; % break; % } % - if (occurrence_sort && list1->occurrence < list2->occurrence % - || hash_sort && list1->hash_value > list2->hash_value) % + if ((occurrence_sort && list1->occurrence < list2->occurrence) % +=09 || (hash_sort && list1->hash_value > list2->hash_value)) % { % *resultp =3D list2; % resultp =3D &list2->next; list2 =3D list1; list1 =3D *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=3D1, 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] ? % +=09 "(str, len)\n" % + " register char *str;\n" % + " register unsigned int len;\n" : % +=09 option[C] ? % +=09 "(str, len)\n" % + " register const char *str;\n" % + " register unsigned int len;\n" : % +=09 "(register const char *str, register unsigned int len)\n"); %=20 % /* Note that when the hash function is called, it has already been ver= ified % that min_key_len <=3D len <=3D 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) %=20 % if (option[DEBUG]) % fprintf (stderr, % - "dup_ptr[%d]: hash_value =3D %d, index =3D %d, coun= t =3D %d\n", % + "dup_ptr[%zd]: hash_value =3D %d, index =3D %d, cou= nt =3D %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] ? % +=09 "(str, len)\n" % + " register char *str;\n" % + " register unsigned int len;\n" : % +=09 option[C] ? % +=09 "(str, len)\n" % + " register const char *str;\n" % + " register unsigned int len;\n" : % +=09 "(register const char *str, register unsigned int len)\n"); %=20 % /* Output the function's body. */ % printf ("{\n"); % Another pseudo-switch invaded. % Modified: head/contrib/gperf/src/options.cc % =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D % --- head/contrib/gperf/src/options.cc=09Wed May 18 21:04:29 2011=09(r2220= 83) % +++ head/contrib/gperf/src/options.cc=09Wed May 18 21:06:20 2011=09(r2220= 84) % @@ -237,7 +237,7 @@ Options::print_options (void) % { % putchar (*arg); % arg++; % - if (*arg >=3D 'A' && *arg <=3D 'Z' || *arg >=3D 'a' && *arg <= =3D 'z') % + if ((*arg >=3D 'A' && *arg <=3D 'Z') || (*arg >=3D 'a' && *arg= <=3D 'z')) % { % putchar (*arg); % arg++; % Another precedence non-problem. Bruce --0-1933995627-1306833235=:1721--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20110531185629.J1721>