Re: [PATCH 01/72] gettext.h: add no-op _() and N_() wrappers

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Feb 20, 2011, 21:11 UTC
Message-ID: <AANLkTinZSpz=H9dt6kRDhOtght_YB4qVpw5WENEYmaQZ@mail.gmail.com>
In-Reply-To: <20110220020130.GA17225@elie>

On Sun, Feb 20, 2011 at 03:01, Jonathan Nieder <jrnieder@gmail.com> wrote:

Show 72 quoted lines

> Ævar Arnfjörð Bjarmason wrote:
>
>> Add a new header called gettext.h which is currently a no-op.
>
> Thanks.  I'd suggest squashing this with patch #2 (#including
> gettext.h in cache.h).
>
>> --- /dev/null
>> +++ b/gettext.h
>> @@ -0,0 +1,9 @@
>> +/*
>> + * Copyright (c) 2010 Ævar Arnfjörð Bjarmason
>
> Is such a simple header file copyrightable?  But I don't mind.
>
>> + *
>> + * This is a skeleton no-op implementation of gettext for Git. It'll
>> + * be replaced by something that uses libintl.h and wraps gettext() in
>> + * a future patch series.
>> + */
>> +#define N_(s) (s)
>
> Might be nice to make this an inline function, for type safety.
>
>> +#define _(s) (s)
>
> This one can't be a function, though, since it needs to transform
> literals to literals.
>
> Some possible tweaks:
>
>  - protect against double inclusion
>  - make _ into a function
>  - add a comment vaguely explaining N_
>  - avoid confusing errors if some other header has pre-defined _.
>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> ---
>  gettext.h |   19 +++++++++++++++++--
>  1 files changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/gettext.h b/gettext.h
> index c68bbe9..2f806cb 100644
> --- a/gettext.h
> +++ b/gettext.h
> @@ -1,3 +1,10 @@
> +#ifndef GETTEXT_H
> +#define GETTEXT_H
> +
> +#ifdef _
> +#error "namespace conflict: '_' is pre-defined?"
> +#endif
> +
>  /*
>  * Copyright (c) 2010 Ævar Arnfjörð Bjarmason
>  *
> @@ -5,5 +12,13 @@
>  * be replaced by something that uses libintl.h and wraps gettext() in
>  * a future patch series.
>  */
> -#define N_(s) (s)
> -#define _(s) (s)
> +
> +static inline const char *_(const char *msgid)
> +{
> +       return msgid;
> +}
> +
> +/* Mark msgid for translation but do not translate it. */
> +#define N_(msgid) (msgid)
> +
> +#endif

Thanks for this. This was raised earlier in the discussion for this series (by you, IIRC). I've considered it, and while I see your point (type safety = good) I think I like my way of doing it better.

The _() macro/function is usually a macro and not a function. The good thing about this is that I can prove that it's a no-op on all compilers, whereas if it's an inline function (stupid) compilers will actually make it into a function, which'll result in overhead, which'll mean I can't advertise this series as a "no-op" anymore.

Check out some of the recent work in perl.git for reference. There's now a probe in perl which check is the compiler can *really* handle inline functions, and if not perl will fall back on using macros.

I'd rather not use inline functions for every string in Git without such a probe, especially since it's for a very marginal gain. Using _() for non-strings isn't going to make it past the list, and if it's used with e.g. multiple arguments compilers will already whine about it.

So that's why I didn't do it. But if everyone else feels strongly about it I'll change my mind, I don't care *that* much about it, but I'd prefer a macro.