Re: [PATCH][PR server/24377] Fix mixing English and system default languages in error messages on Windows

[Eli Zaretskii <eliz at gnu dot org>]

> From: Владимир Мартьянов <vilgeforce@gmail.com>
> Date: Sun, 31 Mar 2019 00:26:30 +0300
> Cc: gdb-patches@sourceware.org
> 
> I'm proposing this patch to translate messages in strwinerror.

Thanks, I have a few comments.

> I didn't check locale name against "C" value because don't know what
> locale to use: English or system default.

The "C" locale is more or less identical to the English locale, not to
the system default.

> +  //retrieve LCID

I believe the convention is to use comments /* like this */.

> +  typedef HRESULT (WINAPI *RFC1766TOLCID)(LCID *pLocale, LPTSTR pszRfc1766);
> +  RFC1766TOLCID Rfc1766ToLcid;

This is an undocumented function.  I think using it is only justified
on XP, because since Vista there's the documented LocaleNameToLCID.
If LocaleNameToLCID is available, we should use it in preference to
Rfc1766ToLcid.

> +  HMODULE hMlang = LoadLibrary("Mlang.dll");
> +  if (hMlang != NULL)
> [...]
> +    FreeLibrary(hMlang);

Instead of loading the DLL every time this function is called, make
hMlang a static variable, and then you need to load the DLL and obtain
the function's entry address only once, the first time the function is
called.

> +      localeName = getenv ("LC_ALL");
> +      if (localeName == NULL || localeName[0] == '\0')
> +      {
> +        /* Next comes the name of the desired category.  */
> +        localeName = getenv ("LC_MESSAGES");
> +        if (localeName == NULL || localeName[0] == '\0')
> +        {
> +          /* Last possibility is the LANG environment variable.  */
> +          localeName = getenv ("LANG");

Bother: do we want to behave like a Posix platform here, or like a
Windows system?  Windows doesn't have LC_MESSAGES as a locale
category.

> +        }
> +      }
> +    }
> +
> +    if (localeName != NULL && localeName[0] != '\0'){   //have something to convert
> +      strncpy(buf, localeName, (COUNTOF (buf)) - 1);
> +
> +      ptr = strchr(buf, '.');		//cut at "."
> +      if (ptr != 0) {
> +        *ptr = 0x00;
> +      }

There are some stylistic issues here: the braces should be on separate
lines, there should be a blank between a function/macro name and the
opening parenthesis, and I believe we use NULL, not zero for a null
pointer.  Also, a single-line block doesn't need to use braces.

Finally, a more general point: I'm not sure I understand the purpose
of this change.  Is the purpose to let users control the language of
the gdbserver system error messages?  If so, would they need to
control that by setting environment variables?  It sounds less
convenient than it could have been, I think.  Why not a GDB variable
instead?

Thanks.