[Toybox] i18n configuration in toy_singleinit

[Rob Landley]

On Wed, Mar 25, 2015 at 12:51 PM, Jim Huang <jserv at 0xlab.org> wrote:
> Hello list,
>
> I just browsed file main.c and found the following:
>
> static void toy_singleinit(struct toy_list *which, char *argv[])
> {
>   ..
>   if (CFG_TOYBOX_I18N) setlocale(LC_ALL, "C"+!!(which->flags &
> TOYFLAG_LOCALE));

The intent is that if one command calls xexec() to run another command
without bouncing off the os and searching $PATH (I.E. toybox calls
itself recursively and opitimizes away the exec, which makes shell
scripts faster once toysh is up and running), then if we _were_
running a command that requires locale support and we switch to a
command that does _not_ expect locale support (I.E. when they do
"sprintf("%*s", len, str)" they are counting _bytes_ not characters
and do not want to overrun a fixed buffer size even if "str" is
arbitrary user-supplied data), then we switch _back_ to the C locale
even though the earlier command did a setlocale("") to whatever the
environment variables say.

So if toybox is built with CFG_TOYBOX_I18N, we always call setlocale()
and set it to "C" or to "" depending on what which->flags says the
command expects. We have to do that because it could be a recursive
call with the locale in who knows what state from the previous
command.

> clang-3.5 complains:
>
> main.c:72:45: warning: adding 'int' to a string does not append to the
> string [-Wstring-plus-int]

That's right, it does not. It gives us a string at a different
starting point. C allows this.

> main.c:72:45: note: use array indexing to silence this warning

Array indexing includes a dereference. We don't want a dereference, we
want an adjusted pointer.

What we're doing is correct, the warning is wrong.

> According to setlocale(3), on  startup  of the main program, the
> portable "C" locale is selected as default.  A program may be made
> portable to all locales by calling setlocale(LC_ALL, "").
>
> It seems that Toybox would like to pass argument "C" or "" to
> setlocale(). Is my understanding right? If so, how can we suppress the
> warning?

The obvious options are:

1)  Patch clang.

2) Find a -fno-stupid-warning option to switch off this specific warning.

3) Declare clang's warning generation "too dumb to live", detect clang
in configure or scripts/make.sh, and remove -Wall from $CFLAGS for it.

I'm reluctant to change the code to placate clang if the code isn't
wrong and clang is. If clang is an open source project, presumably the
compiler can be fixed.

I could add a comment that clang generates a broken warning here and
the problem is clang, not our code? (I would prefer to add that
instead of a typecast because the comment is more likely to be noticed
and removed if the compiler was fixed, typecasts tend to persist with
nobody remembering _why_. Plus unnecessary typecasts hide real bugs,
and occasionally cause them.)

However, given that our code uses the "str+!!(x)" trick in a number of
places (the !! forces a true/false value to be either 0 or 1),
commenting them all would be silly...

Rob

 1427397902.0