Re: [PATCH v2] gettext.h: add parentheses around N_ expansion if supported

From: Kyle J. McKay <mackyle@gmail.com>
Date: Jan 9, 2015, 04:55 UTC
Message-ID: <61CC69D2-A0F2-4BE2-B37A-F1D8FE19C45A@gmail.com>
In-Reply-To: <xmqqa91tf5ql.fsf@gitster.dls.corp.google.com>

On Jan 8, 2015, at 11:10, Junio C Hamano wrote:

Show 8 quoted lines

> "Kyle J. McKay" <mackyle@gmail.com> writes:
>
>> For now only __GNUC__ is tested which covers both gcc and clang
>> which should result in early detection of any adjacent N_ macros.
>
> I didn't check the list of -W options, but if there were a way to
> tell gcc to stick to the C standard in a more strict way than its
> default, wouldn't this patch start causing trouble?

With this test program:

-----BEGIN TEST.C----- #include <stdio.h>

#define msg1(x) x #define msg2(x) (x)

static const char *const a1[] = {
	msg1("hi"),
	msg2("bye") /* line 8 */
};

static const char s1[] = msg1("hi");

static const char s2[] = msg2("bye"); /* line 13 */

int main()
{
	puts(a1[0]);
	puts(a1[1]);
	puts(s1);
	puts(s2);
	return 0;
}
-----END TEST.C-----

gcc, (but not clang) emits a warning (it still compiles just fine) when -pedantic is used:

   test.c:13: warning: array initialized from parenthesized string  
constant

However, none of -ansi, -Wall or -Wextra trigger that warning.

Neither does using -ansi -Wall -Wextra together cause a warning to be emitted (by either gcc or clang).

Note that line 8 never causes a problem (nor should it), only line 13 is in question.

After a quick read-through of the -Wxxx options there does not appear to be a separate -Wxxx option to get that particular warning.

And compiling Git with -pedantic spits out a LOT of warnings (over  
7200) even before making the "(msgid)" change so I don't think there's  
an issue as apparently -pedantic is not normally used to compile Git.

Note that Git will not compile with gcc using -ansi (unless you add - Dinline=__inline__) and the change does not cause any new warnings to be emitted with -ansi (after adding the needed -Dinline=__inline__) since -pedantic is required for the "parenthesized string constant" warning.

I'm not super attached to this change, it's just that it seems to me that translation support for Git is a scarce resource. I'm guessing that when considering the 7 complete translations (bg, ca, de, fr, sv, vi and zh_CN) the average number of translators per language is in the low single digits. So I hate to see unnecessary translation churn, not when it can be so easily prevented.

-Kyle

Show 37 quoted lines

>> Although the necessary #ifdef makes the header less elegant,
>> the benefit of avoiding propagation of a translation-marking
>> error to all the translation teams thus creating extra work
>> for them when the error is eventually detected and fixed would
>> seem to outweigh the minor inelegance the #ifdef introduces.
>>
>> Signed-off-by: Kyle J. McKay <mackyle@gmail.com>
>> ---
>> gettext.h | 14 +++++++++++++-
>> 1 file changed, 13 insertions(+), 1 deletion(-)
>>
>> diff --git a/gettext.h b/gettext.h
>> index 7671d09d..80ec29b5 100644
>> --- a/gettext.h
>> +++ b/gettext.h
>> @@ -62,7 +62,19 @@ const char *Q_(const char *msgid, const char  
>> *plu, unsigned long n)
>> 	return ngettext(msgid, plu, n);
>> }
>>
>> -/* Mark msgid for translation but do not translate it. */
>> +/* Mark msgid for translation but do not translate it.
>> + *
>> + * In order to prevent accidents where two adjacent N_ macros
>> + * are mistakenly used, this macro is defined with parentheses
>> + * when the compiler is known to support parenthesized string
>> + * literal assignments.  This guarantees a compiler error in
>> + * such a case rather than a silent conjoining of the strings
>> + * by the preprocessor which results in translation failures.
>> + */
>> +#ifdef __GNUC__
>> +#define N_(msgid) (msgid)
>> +#else
>> #define N_(msgid) msgid
>> +#endif
>>
>> #endif