This is the mail archive of the
mailing list for the glibc project.
Re: Cross-rpcgen patch, version 6
- From: Roland McGrath <roland at hack dot frob dot com>
- To: "Joseph S. Myers" <joseph at codesourcery dot com>
- Cc: libc-alpha at sourceware dot org
- Date: Wed, 9 May 2012 15:24:07 -0700 (PDT)
- Subject: Re: Cross-rpcgen patch, version 6
- References: <Pine.LNX.email@example.com>
I don't like the idea of touching Makerules for something that is
neither really general-purpose nor used in more than one place.
This change can be entirely confined to sunrpc/, so please do that.
As I said before, I think we should have first-class machinery for
compiling nontrivial build-host programs. Please file a bug for
that, and give sunrpc/Makefile comments that refer to that bug's URL
(or just BZ# if you prefer, though IMHO a URL is more handy).
> # Tell rpcgen where to find the C preprocessor.
> -rpcgen-cmd = CPP='$(CC) -E -x c-header' $(built-program-cmd) -Y ../scripts
> +rpcgen-cmd = CPP='$(CC) -E -x c-header' $(built-program-file) -Y ../scripts
As Carlos mentioned, the meaning of built-program-file is not obvious.
It also reads as odd that the sole comment for the line is about the
CPP=... portion. (Not that either of these is really the fault of your
change.) Please amend the comment to something like:
# How we run rpcgen to generate sources and headers in the rules below.
# Setting CPP tells it how to run the C preprocessor correctly. Note
# that $(built-program-file) requires that the just-built cross-rpcgen
# binary be the second dependency listed in each rule using rpcgen-cmd.
> +#ifdef IS_IN_build
> +#define _(X) (X)
> +#define _libc_intl_domainname "libc"
Please give these macros some comments explaining what they're doing.
Since the _ definition means that gettext will never be used, I think it
would be more clear and clean (and incidentally trivially more
efficient) to define away textdomain rather than _libc_intl_domainname.
If it's really significantly easier to leave the textdomain call intact
for some reason, then I think the string should be something like
"***UNUSED***" that makes it more obvious that this is just a cruft
workaround rather than something meaningful for correct operation.