Re: PE+ and new COFF format for x86_64 target for XP64 and Vista binaries

[Pedro Alves <pedro_alves at portugalmail dot pt>]

Hi Kai,

Kai Tietz wrote:
> Hey,
>
> Finally I did the merge to the current CVS version. Additionally I 
changed
> pep_dll.c to inherit the pe_dll.c file to avoid double source code.
> By this merge I fixed - by side-effect - a memory out-of-bounds 
access for
> idata relocation section in pe_dll.c (allocating 4-bytes but 
memset'ing 8
> bytes).
> The changelogs are now splitted and the testcases adjusted for the new
> target. I removed some escaped debug prints, which lead under cygwin
> environment to compiler warnigns in the ld-module.
>

A few more tips that may get your work into CVS faster.

- The ChangeLog entries aren't supposed to be put in the patch itself.
They should be placed in the email instead. Check the mail archives for
reference.

- You should really consider submitting this piece by piece. I would
have some of comments to the code, but the patch being so big, puts me
off. At least please split the bfd/gas/ld/binutils parts. I would start
with bfd first. I know its more work for you, but it should be less work
for the maintainers, and that's who you want to catch attention from.
Keep in mind, they are very busy with other stuff. The easier you make
it for them, the best for you.

- There are still some big files that you duplicated.
Please describe why you needed to duplicate them when you submit your
patches. Maybe the limitations you found in the originals could be
tackled, instead of just duplicating them. Duplicate code just means
more maintainance for you. If a bug gets fixed in the original, well,
you know what I mean.
For example the ld/emultempl/pe.em file is already a shell script so
the code duplication is minimized, Couldn't it be adapted instead of
duplicated? There is already a copy of it there (beos.em), and in my
opinion another one would be bad.

- On lots of places you touch code just to change the indenting. It only
makes the patch longer, and harder to review.

- When you touch code that is common with other archs, you should at
least run the testsuite in one of those other archs,
and say you did so in patch submission. That brings up the confidence
the patch is ok for everybody else.

Just trying to be helpful,
Cheers,
Pedro Alves