This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
RE: Question about windres extension and a message compiler for windres.
- From: Kai Tietz <Kai dot Tietz at onevision dot com>
- To: "Dave Korn" <dave dot korn at artimi dot com>
- Cc: binutils at sourceware dot org, "'Danny Smith'" <dannysmith at clear dot net dot nz>
- Date: Mon, 21 May 2007 10:13:07 +0200
- Subject: RE: Question about windres extension and a message compiler for windres.
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