From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 26940 invoked by alias); 15 Mar 2010 11:04:00 -0000 Received: (qmail 26797 invoked by uid 22791); 15 Mar 2010 11:03:59 -0000 X-SWARE-Spam-Status: No, hits=-1.9 required=5.0 tests=AWL,BAYES_00,SPF_FAIL X-Spam-Check-By: sourceware.org Received: from mx20.gnu.org (HELO mx20.gnu.org) (199.232.41.8) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Mon, 15 Mar 2010 11:03:55 +0000 Received: from mail.codesourcery.com ([38.113.113.100]) by mx20.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1Nr85Q-0005GE-S4 for gcc-patches@gcc.gnu.org; Mon, 15 Mar 2010 07:03:53 -0400 Received: (qmail 19300 invoked from network); 15 Mar 2010 11:03:51 -0000 Received: from unknown (HELO digraph.polyomino.org.uk) (joseph@127.0.0.2) by mail.codesourcery.com with ESMTPA; 15 Mar 2010 11:03:51 -0000 Received: from jsm28 (helo=localhost) by digraph.polyomino.org.uk with local-esmtp (Exim 4.69) (envelope-from ) id 1Nr85O-0000BD-38; Mon, 15 Mar 2010 11:03:50 +0000 Date: Mon, 15 Mar 2010 11:42:00 -0000 From: "Joseph S. Myers" To: Shujing Zhao cc: GCC Patches , Paolo Carlini Subject: Re: [PATCH PR/42686] Align the help text output In-Reply-To: <4B9DFA37.9010303@oracle.com> Message-ID: References: <4B863559.4000407@oracle.com> <4B976D51.8010705@oracle.com> <4B9A1E65.4080506@oracle.com> <4B9DFA37.9010303@oracle.com> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII X-detected-operating-system: by mx20.gnu.org: GNU/Linux 2.6, seldom 2.4 (older, 4) Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org X-SW-Source: 2010-03/txt/msg00578.txt.bz2 On Mon, 15 Mar 2010, Shujing Zhao wrote: > +/* Return the number of bytes when converts the wide character WC > + to its multibyte representation. */ "Return the number of bytes in the multibyte representation of the wide character WC.". > + char *pmb = (char *)xmalloc (MB_CUR_MAX); Missing space between "(char *)" and "xmalloc". > +/* Return the length of MSGSTR that can be printed within the width ROOM. */ > + > +unsigned int > +get_aligned_len (const char *msgstr, unsigned int room, > + unsigned int remaining) You need to say what REMAINING is. > @@ -1158,12 +1158,31 @@ wrap_help (const char *help, The comment at the top of this function should be changed to make explicit that ITEM_WIDTH is a count of bytes (if that is indeed the case). > + if (item_width == item_col_width > + || item_width == 0) > + printf (" %-*.*s", col_width, item_width, item); > + /* Handle the item that includes wide character. */ > + else I think the code would be a lot clearer with fewer conditionals. Why do you need to handle specially the case where item_width == item_col_width? Can't you just use the logic below unconditionally? > + { > + printf (" %s", item); > + /* Output a certain number of spaces for alignment. */ > + if (item_col_width < col_width) > + { > + unsigned int n_spaces = col_width - item_col_width; > + const char *space = " "; > + for (i = 0; i < n_spaces; i++) > + printf ("%s", space); You don't actually need a loop here (given a number of spaces representable as an "int", which I think you can assume); you can use "%*s" as a format with an empty string argument to have printf output the number of spaces you computed. > + if (len != gcc_gettext_width (help)) > + len = get_aligned_len (help, room, remaining); Again, I'd like to avoid conditionals if possible. > + for (i = 0; help[i]; i++) > + { > + if (i >= room && len != remaining) > + break; > + if (help[i] == ' ') > + len = i; > + else if ((help[i] == '-' || help[i] == '/') > + && help[i + 1] != ' ' > + && i > 0 && ISALPHA (help[i - 1])) > + len = i + 1; > + } You now have two copies of line-splitting logic, one in get_aligned_len and one in opts.c. It would be better for the logic to be just in get_aligned_len. You might still need two copies - the one using wide character functions and a simpler one where they aren't available - but having both in the same file would be better than sometimes using logic from one file and sometimes from another file. (This would mean that intl.h no longer has a dummy definition of get_aligned_len; there would always be a real function that works out a suitable point to break a line.) > + buf = (char *)alloca (len + 1); Missing space in cast again. -- Joseph S. Myers joseph@codesourcery.com