[patch]: dllwrap ported for x64 and support for new --[no-]leading-underscore option

Dave Korn dave.korn.cygwin@googlemail.com
Wed Nov 4 15:03:00 GMT 2009


Kai Tietz wrote:

> Tested for x86_64-pc-mingw32, i686-pc-mingw32, and for i686-cygwin. Ok
> for apply?

  Not quite.  What is all this stuff about the "host"?  You appear to be
referring to the cpu type of the *target* by it, rather than any property of
the host machine, and of course I don't suppose you actually mean to imply
that the output should depend on properties of the system on which the
toolchain is running, rather than the properties of the system on which the
output executable images will be run.

  The terms "host" and "target" are already well defined in the gnu build
system and we should not muddy the waters like this.  Please rename host_type
and which_host to completely remove the word 'host' in all its forms; I would
suggest you replace it by "target_cpu" or similar.

  Other than that:

> +      const char *preFix = (is_leading_underscore != 0 ? "_" : "");
> +      const char *postFix = "";
> +      const char *nameEntry;

  There is no camel-case anywhere else in this file that I could see.  Please
don't gratuitously introduce new and inconsistent coding style just to satisfy
a personal preference.  I wouldn't complain if the file was already full of
mixed styles, but it isn't yet, so let's not start.

  The rest of it looks ok.  There's one place the formatting looked a little
peculiar:

> +  else if (!strncmp (target, "x86_64", 6)
> +           || !strncmp (target, "athlon64", 8)
> +	   || !strncmp (target, "amd64", 5))
> +    which_host = X64_HOST;
> +  else if (target[0] == 'i' && (target[1] >= '3' && target[1] <= '6')

but I won't worry about formatting issues until we see your respin without the
"host" confusion.  Thanks for your patience.

    cheers,
      DaveK




More information about the Binutils mailing list