RE: Question about windres extension and a message compiler for windres.

["Dave Korn" <dave dot korn at artimi dot com>]

On 20 May 2007 11:57, Danny Smith wrote:

>> -----Original Message-----
>> From: binutils-owner On Behalf Of Kai Tietz
>> Sent: Wednesday, 16 May 2007 12:43 a.m.

>> Hello,
>> 
>> I am sorry for querying, but I am really interested in. Is somebody
>> reviewing or commenting on this windres extension  patch ?
>> 
> 
> 
> With your patch I get these new errors in windres testsuite

  I see these errors too.

  Also, to comment on the patch: I'm not a maintainer, but I gave it a look over and it looked pretty good to me.  There are a few minor issues I have queries about:

  I see that you've done a more-or-less mechanical replacement of struct res_XXX with a typedef rc_res_xxx throughout; it would have been easier to review if that had been a separate patch and the functional changes were a second patch relative to that.  (The get_16/32->windres_get_16/32 and changing all the functions to pass around a windres_bfd arg instead of using globals could also have been factored out as a separate patch, I think).  Anyway, ploughing on:

Index: binutils/rcparse.y
===================================================================
RCS file: /cvs/src/src/binutils/rcparse.y,v
retrieving revision 1.23
diff -b -u -r1.23 rcparse.y
--- binutils/rcparse.y	26 Apr 2007 14:46:59 -0000	1.23
+++ binutils/rcparse.y	11 May 2007 12:44:00 -0000
@@ -1,7 +1,8 @@
 %{ /* rcparse.y -- parser for Windows rc files
-   Copyright 1997, 1998, 1999, 2000, 2001, 2002, 2003, 2005, 2007
+   Copyright 1997, 1998, 1999, 2000, 2001, 2002, 2003, 2005
    Free Software Foundation, Inc.
    Written by Ian Lance Taylor, Cygnus Support.
+   Extended by Kai Tietz, Onevision.
 
    This file is part of GNU Binutils.
 

Lost a year from the copyright info!  You also did this in binutils/rcparse.y, binutils/resbin.c, binutils/rescoff.c, binutils/resrc.c, binutils/resres.c, binutils/winduni.c.  And you should update the copyright info in the other files touched, binutils/windres.h, binutils/winduni.c, and the new files rclex.c and windint.h.

  In binutils/rcparse.y:

@@ -50,56 +52,62 @@
 /* These are used when building a control.  They are set before using
    control_params.  */
 
-static unsigned long base_style;
-static unsigned long default_style;
-static unsigned long class;
-static struct res_id res_text_field;
+static bfd_vma base_style;
+static bfd_vma default_style;
+static rc_res_id class;
+static rc_res_id res_text_field;


  I don't understand changes like this.  A bfd_vma represents a memory address.  The base_style and default_style are IIUIC bitmasks of windows style flags.  It seems very wrong to use such an unrelated type.

   struct
   {
-    unsigned short on;
-    unsigned short off;
+    bfd_vma on;
+    bfd_vma off;
   } memflags;

  This is even more strange; it's a completely different size.  Is the use of bfd_vma something to do with your endianness changes?

   const char *s;
   struct
   {
-    unsigned long length;
+    bfd_vma length;
     const char *s;
   } ss;
+  unichar *uni;
+  struct
+  {
+    bfd_vma length;
+    const unichar *s;
+  } suni;

  What's wrong with unsigned long (or perhaps even better, size_t) for these?


+%type <toobar_item> toolbar_data
             . . . . 
+  rc_toolbar_item *toobar_item;

  Should be 'toolbar_item', not 'toobar_item'.


binutils/resbin.c

+static void get_version_header (windres_bfd *, const bfd_byte *, bfd_vma, const char *, unichar **, bfd_vma *, bfd_vma *, bfd_vma *, bfd_vma *);

  Rather a long line (146 chars), should be wrapped.

+	case RT_TOOLBAR:
+	  return  bin_to_res_toolbar (wrbfd, data, length);
               ^^
  Excess space nitpick.

-  struct res_resource *r;
+  rc_res_resource *r;
 
-  r = (struct res_resource *) res_alloc (sizeof *r);
+  r = (rc_res_resource *) res_alloc (sizeof (rc_res_resource));

  There are a lot of changes like this.  I would have preferred if you kept the "sizeof *r" construct; it's slightly less work to maintain if the type of r is ever redefined.  It's a trivial consideration, however.

/usr/build/src-binutils $ grep '! ' nwindres.txt  | grep ^+
+         if (! first)
+         if (! first)
+      if (! ISPRINT (data[i]) && data[i] != '\n' && !(data[i] == '\r' && (i + 1) < length && data[i + 1] == '\n') && data[i] != '\t'
+  if (! probe_binary (&wrbfd, flen))
+    set_windres_bfd_endianess (&wrbfd, ! target_is_bigendian);
+  if (! bfd_set_section_flags (abfd, sec,
+  if (! bfd_set_section_size (abfd, sec, (sec_length + 3UL) & ~3UL))
+      if (! bfd_set_section_contents (WR_BFD(wrbfd), WR_SECTION(wrbfd), data, off, length))
+      if (! bfd_get_section_contents (WR_BFD(wrbfd), WR_SECTION(wrbfd), data, off, length))
+  if (! unichar_isascii (u, len))
/usr/build/src-binutils $ grep '![^ =]' nwindres.txt | grep ^+
+      if (!dialogex)
+         if (!dialogex)
+         if (!dialogex)
+      if (!dialogex)
+      if (!menuex)
+      if (! ISPRINT (data[i]) && data[i] != '\n' && !(data[i] == '\r' && (i + 1) < length && data[i + 1] == '\n') && data[i] != '\t'
+       && !(data[i] == 0 && (i + 1) != length))
+  if (length > 80 && !has_nl)
+         if (!first)
+  if (!src)
+  if (!flen)
+  if ((reshdr.header_size != 0x20 && !target_is_bigendian)
+  if ((reshdr.header_size != 0x20 && !target_is_bigendian)
+  if (!target_vec)
+    if (!def_target_arch)
+  if (!abfd)
+  if (rdmode && !bfd_check_format (abfd, bfd_object))
+  if (!wrbfd)
+    if (!is_bigendian)
+  if (!wrbfd)
+    if (!abfd)
+    if (!sec)
+  if (!p)
+  if (!p)
+  if (!p)
+  if (!fmt)
+  if (!unicode)
+  if (!r)
/usr/build/src-binutils $

  Consistency: sometimes you put a space after the ! operator, sometimes not.  I see that the old code isn't entirely consistent, but a quick grep suggests:

/usr/build/src-binutils $ grep '![^= ]' binutils/*  | wc -l
364
/usr/build/src-binutils $ grep '! ' binutils/* | wc -l
1495
/usr/build/src-binutils $

that with-a-space is the more common idiom.

binutils/resres.c:

+  if ((reshdr.header_size != 0x20 && !target_is_bigendian)
+      || (reshdr.header_size != 0x20000000 && target_is_bigendian))

  Magic numbers are bad :-( but there are an awful lot of them in windres already.  In particular all the calls to get/set_16/32 seem to use hardcoded constants where maybe offsetof() and sizeof() would be better.

+  if (!wrbfd)
+    fatal ("set_windres_bfd_endianess: NULL windres_bfd.");

  Consider whether these sort of checks could be more simply replaced by asserts.  Calling a function with a NULL arg where a pointer should be supplied is a coding error; diagnostics are intended primarily for end-users, to report errors in the commands and/or input files they have supplied.


    cheers,
      DaveK
-- 
Can't think of a witty .sigline today....