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

[Kai Tietz <Kai dot Tietz at onevision dot com>]

Hello,

binutils-owner@sourceware.org wrote on 20.05.2007 18:29:34:

> 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.

I had an typo in resres.c storing instead of 0xffff just 0xfff. Upps...
It will be fixed in the updated version.

>   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:

> 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.

Yes, I fixed it, too. The dates were added while I patched those files and 
I missed to merge this changes.

>   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.

I used the type bfd_vma for preventing people to think that rc_res_(XXX) 
structures are anyhow related to binary layout of these structures. 
Because this leads in fact to problems for different architectures with 
different base type sizes.

>   This is even more strange; it's a completely different size.  Is 
> the use of bfd_vma something to do with your endianness changes?
>   What's wrong with unsigned long (or perhaps even better, size_t) for 
these?
See above. A very good point for changing it, isn't it ?

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

 Ok, I will go through source and wrap those really long lines. I use a 
terminal with about 200 characters width and I didn't noticed that. 8)

>   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.

I dislike the variant of sizeof (*r), because to know which kind (*r) is I 
need to look at the variable declaration. But may this is just for me a 
common habit to choose this kind ...

>   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:

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

I agree, I try to find them all and adjust it with space. I personally 
prefer the variant without, but ...

> +  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() wouldbe 
better.

May a task for next update ? I think it would be also a good idea to add 
(for windows arch) to windres the support of code pages, but there is no 
equivalent for unix without using a library as iconv ?

> +  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.

Agreed.


Cheers,
 i.A. Kai Tietz

|  (\_/)  This is Bunny. Copy and paste Bunny
| (='.'=) into your signature to help him gain
| (")_(") world domination.

------------------------------------------------------------------------------------------
  OneVision Software Entwicklungs GmbH & Co. KG
  Dr.-Leo-Ritter-StraÃe 9 - 93049 Regensburg
  Tel: +49.(0)941.78004.0 - Fax: +49.(0)941.78004.489 - www.OneVision.com
  Commerzbank Regensburg - BLZ 750 400 62 - Konto 6011050
  Handelsregister: HRA 6744, Amtsgericht Regensburg
  KomplementÃrin: OneVision Software Entwicklungs Verwaltungs GmbH
  Dr.-Leo-Ritter-StraÃe 9 â 93049 Regensburg
  Handelsregister: HRB 8932, Amtsgericht Regensburg - GeschÃftsfÃhrer: 
Ulrike DÃhler, Manuela Kluger