Re: [PATCH] fix new format when printing multiple lines

[Jim Meyering]

Hi Sami,

Thanks for working on this.
After you pushed that, I noticed a couple things that
I'd do differently.  Since the new print_box function
simply prints the specified message, that message
parameter should be a "const" pointer.  The only reason
we can't simply add the "const" is because the current
implementation happens to modify it, incidentally.
It uses wcstok to extract the lines, and that function
writes into the string being parsed.  It is better both
for efficiency and aesthetics to avoid use of that function,
just as it's good to avoid use of strtok.  Instead, I would
suggest to use wcschr.  Then the parameter can be the
more expected "const" pointer.  Also, with sizeof, it is
more maintainable to use xmalloc(sizeof *VAR) than
xmalloc(sizeof TYPE), since that avoids the risk of
error, e.g., when someone changes the type of VAR from
TYPE to NEW_TYPE and forgets to update this use of the
type name.

I've attached a suggested starting point (doesn't even
compile warning-free yet -- needs the wcschr change
I mentioned), in case you feel like working on this.

Thanks,
Jim

diff --git a/src/hello.c b/src/hello.c
index 9c27234..d6d2d53 100644
--- a/src/hello.c
+++ b/src/hello.c
@@ -40,7 +40,7 @@ typedef enum
 /* Forward declarations.  */
 static void print_help (void);
 static void print_version (void);
-static void print_box (wchar_t * mb_greeting);
+static void print_box (const wchar_t *mb_greeting);
 static void print_frame (const size_t len);

 int
@@ -133,8 +133,8 @@ main (int argc, char *argv[])

 /* New format message in box.  */

-void
-print_box (wchar_t * greeting)
+static void
+print_box (const wchar_t *greeting)
 {
   wchar_t *ignored;
   size_t longest_line = 0;
@@ -147,7 +147,7 @@ print_box (wchar_t * greeting)
   };
   struct parts *first, *p;

-  first = xmalloc (sizeof (struct parts));
+  first = xmalloc (sizeof *first);
   first->next = NULL;
   p = first;

@@ -166,7 +166,7 @@ print_box (wchar_t * greeting)
       p->len = len_tabs - i;
       if (longest_line < len_tabs)
        longest_line = len_tabs;
-      p->next = xmalloc (sizeof (struct parts));
+      p->next = xmalloc (sizeof *p->next);
       p = p->next;
       p->str = wcstok (NULL, L"\n", &ignored);
     }