Discussion:
[PATCH] fix ps.c build
(too old to reply)
enh
2015-04-18 20:43:00 UTC
Permalink
Fix ps.c build.

external/toybox/toys/pending/ps.c:315:21: error: address of array
'field->title' will always evaluate to 'true'
[-Werror,-Wpointer-bool-conversion]
if (!field->title) strcpy(field->title, typos[field->which]);
~~~~~~~~^~~~~

diff --git a/toys/pending/ps.c b/toys/pending/ps.c
index cb0f32c..29111d5 100644
--- a/toys/pending/ps.c
+++ b/toys/pending/ps.c
@@ -312,7 +312,7 @@ void ps_main(void)
if (j!=2) break;
}
if (i == ARRAY_LEN(typos)) error_exit("bad -o %.*s", end-type, type);
- if (!field->title) strcpy(field->title, typos[field->which]);
+ if (!*field->title) strcpy(field->title, typos[field->which]);
dlist_add_nomalloc((void *)&TT.fields, (void *)field);
}
}
Isaac Dunham
2015-04-19 01:48:14 UTC
Permalink
Post by enh
Fix ps.c build.
external/toybox/toys/pending/ps.c:315:21: error: address of array
'field->title' will always evaluate to 'true'
[-Werror,-Wpointer-bool-conversion]
if (!field->title) strcpy(field->title, typos[field->which]);
~~~~~~~~^~~~~
diff --git a/toys/pending/ps.c b/toys/pending/ps.c
index cb0f32c..29111d5 100644
--- a/toys/pending/ps.c
+++ b/toys/pending/ps.c
@@ -312,7 +312,7 @@ void ps_main(void)
if (j!=2) break;
}
if (i == ARRAY_LEN(typos)) error_exit("bad -o %.*s", end-type, type);
- if (!field->title) strcpy(field->title, typos[field->which]);
+ if (!*field->title) strcpy(field->title, typos[field->which]);
dlist_add_nomalloc((void *)&TT.fields, (void *)field);
I think you might want to hold off on updating ps; right now, it's
the "current cleanup project", which means that it's likely to break at
random, repeatedly.
If you do update before that finishes, check that it works.

Thanks,
Isaac Dunham
Rob Landley
2015-04-19 03:21:39 UTC
Permalink
Post by Isaac Dunham
Post by enh
Fix ps.c build.
external/toybox/toys/pending/ps.c:315:21: error: address of array
'field->title' will always evaluate to 'true'
[-Werror,-Wpointer-bool-conversion]
if (!field->title) strcpy(field->title, typos[field->which]);
~~~~~~~~^~~~~
diff --git a/toys/pending/ps.c b/toys/pending/ps.c
index cb0f32c..29111d5 100644
--- a/toys/pending/ps.c
+++ b/toys/pending/ps.c
@@ -312,7 +312,7 @@ void ps_main(void)
if (j!=2) break;
}
if (i == ARRAY_LEN(typos)) error_exit("bad -o %.*s", end-type, type);
- if (!field->title) strcpy(field->title, typos[field->which]);
+ if (!*field->title) strcpy(field->title, typos[field->which]);
dlist_add_nomalloc((void *)&TT.fields, (void *)field);
I think you might want to hold off on updating ps; right now, it's
the "current cleanup project", which means that it's likely to break at
random, repeatedly.
If you do update before that finishes, check that it works.
Yeah, sorry. It's about 75% done, but the last time I got any
significant work on it was the plane ride over here. (My flight back
is shortly after midnight tuesday morning, I might get it finished on
the way over. Dunno.)

I intend to finish and promote it this month, for what that's worth.
(I've got another 2 weeks...)

Rob
enh
2015-04-19 04:15:27 UTC
Permalink
Post by Rob Landley
Post by Isaac Dunham
Post by enh
Fix ps.c build.
external/toybox/toys/pending/ps.c:315:21: error: address of array
'field->title' will always evaluate to 'true'
[-Werror,-Wpointer-bool-conversion]
if (!field->title) strcpy(field->title, typos[field->which]);
~~~~~~~~^~~~~
diff --git a/toys/pending/ps.c b/toys/pending/ps.c
index cb0f32c..29111d5 100644
--- a/toys/pending/ps.c
+++ b/toys/pending/ps.c
@@ -312,7 +312,7 @@ void ps_main(void)
if (j!=2) break;
}
if (i == ARRAY_LEN(typos)) error_exit("bad -o %.*s", end-type, type);
- if (!field->title) strcpy(field->title, typos[field->which]);
+ if (!*field->title) strcpy(field->title, typos[field->which]);
dlist_add_nomalloc((void *)&TT.fields, (void *)field);
I think you might want to hold off on updating ps; right now, it's
the "current cleanup project", which means that it's likely to break at
random, repeatedly.
If you do update before that finishes, check that it works.
Yeah, sorry. It's about 75% done, but the last time I got any
significant work on it was the plane ride over here. (My flight back
is shortly after midnight tuesday morning, I might get it finished on
the way over. Dunno.)
I intend to finish and promote it this month, for what that's worth.
(I've got another 2 weeks...)
yeah, for now i've taken ps out of the Android .config. we didn't have
a symbolic link to it anyway, and although it's making progress, it's
not currently very likely that anyone would want to "toybox ps".

here's the current list of things from pending that get built into the
AOSP toybox binary. ones marked with * get a symbolic link too.

toys/pending/dd.c
toys/pending/expr.c *
toys/pending/hwclock.c *
toys/pending/more.c *
toys/pending/pgrep.c *
toys/pending/netstat.c *
toys/pending/route.c *
toys/pending/tar.c *
toys/pending/top.c
toys/pending/tr.c *
toys/pending/traceroute.c

those with links are either demonstrably better than what we had, or
things we just didn't have before. so although rob worries, it's not
like we're shipping all of pending and relying on it :-)
Rob Landley
2015-04-19 05:36:51 UTC
Permalink
Post by enh
Post by Rob Landley
I intend to finish and promote it this month, for what that's worth.
(I've got another 2 weeks...)
yeah, for now i've taken ps out of the Android .config. we didn't have
a symbolic link to it anyway, and although it's making progress, it's
not currently very likely that anyone would want to "toybox ps".
here's the current list of things from pending that get built into the
AOSP toybox binary. ones marked with * get a symbolic link too.
toys/pending/dd.c
toys/pending/expr.c *
toys/pending/hwclock.c *
toys/pending/more.c *
toys/pending/pgrep.c *
toys/pending/netstat.c *
toys/pending/route.c *
toys/pending/tar.c *
toys/pending/top.c
toys/pending/tr.c *
toys/pending/traceroute.c
those with links are either demonstrably better than what we had, or
things we just didn't have before. so although rob worries, it's not
like we're shipping all of pending and relying on it :-)
I'm cleaning up as fast as I can!

Speaking of which, here's a proposed getprop.c cleanup that I haven't
got a test environment for. The attached patch is infrastructure
changes, then the changed getprop.c file, and here's the notes from
cleanup:

---------

Proposed changes! (I'm not checking this in because I need your review
before doing that, I don't have your build environment or domain
expertise here.)

Instead of an #ifdef, let's add a "depends on TOYBOX_ON_ANDROID" so
you can only select the sucker when we're building on an android
system. (Meaning I can't test this, but I couldn't anyway because I
didn't have the header. I need to chop a build environment out of the
AOSP build...) [todo: teach "make change" about probed symbols.]

Globals go in GLOBALS(), this includes "properties" here.

Google says PROPERTY_VALUE_MAX is 92, sizeof(toybuf) is 4096, so let's
just use that.

Since properties is an array of pointers to 2 entry structures, just
put the structs inline in the array. Since the name is the first entry
and you can typecast a struct pointer to a pointer to its first entry
(C99 says so), we can use strcmp() directly in qsort, so that function
goes away.

(Except we have to wrap it anyway to do an extra dereference. [See
GIANT_RANTY_FOOTNOTE at the bottom] So I added a qstrcmp() function to
lib/lib.c.)

Also the free() function is just free() the array at the end so that function
goes away.

Also in add_property we realloc() with 32 more slots each time size is
a multiple of 32, so we don't need a separate capacity counter. (Since 0
is an even multiple of 32 we alloc the initial data the same way.)

I'm assuming that property_list() will set errno on a failure, and thus used
perror_exit(). (This is a questionable assumption I can't test, hence the shout
out here.)

About changing the xstrdup() prototype: Toybox doesn't use "const" because
I've never encountered a compiler that does anything intelligent with it
(what's it gonna do, put stack variables in a read-only segment?) and we
otherwise spend a huge amount of time getting it all to line up with no
actual benefit. There's a cleanup.html entry to this effect:
http://landley.net/toybox/cleanup.html#advice

(I'm aware property_list() wants const in its prototype, and am hitting that
with (void *) which is ugly but still fewer typecasts than before.)

And looking at it: we don't really need the struct at all because it's just
pairs of char * with alternating meanings, that's simple enough to just do
inline. (This is a "six of one, half dozen of another" thing.)

Query: why size_t here? Is more than 2 billion of them likely to come up?
(Left it alone, but...)

Rob

P.S. GIANT_RANTY_FOOTNOTE:

qsort() really should have standard sort functions for things like
strcmp() on an array of pointers (or structs whose first member is a
pointer, which work the same way as far as qsort() is concerned). The
man page even has EXAMPLE CODE showing how to do this, and yet it's
not in libc and everybody needs to do their own gratuitous wrapper
function.

It _does_ say we can use alphasort(), which is wrong. That says it
wraps strcoll() which is equivalent to strcmp() in the "C" locale
(which we're in if we didn't feed TOYFLAG_LOCALE to the flags up top,
but I dunno if bionic actually implements that, even though it's in
C99. But that's not the wrong part, the wrong part is that when I
actually _try_ it I get incompatible pointer type because the man page
says it's alphasort(const void *a, const void *b) and the compiler is
saying "oh no, those arguments are struct dirent **"...

Ah, it's because the Posix Committee has its head up its ass:
http://austingroupbugs.net/view.php?id=142

So posix-2008 is forcing people to use a typecast to get historical
behavior _in_the_name_of_type_safety_. That's just hilarious... (Ok,
looking deeper the Solaris guys broke this, then Jorg Schilling pushed
it upstream into posix, and it's up to _me_ to email Michael Kerrisk
to fix the man page _7_years_ later. And no, the solaris guys _really_
broke this so it doesn't work anymore even if you typecast. So the man
page needs fixing and the posix guys need somebody to mail them a box
of live stinging insects with a card that plays "seasons in the sun"
when you open it and doesn't stop when you close it again until the
battery runs down. Use a lithium battery.)

Anyway, I added a lib.c function for this. I was TEMPTED to add a
portability.c function and just declare posix's actions on this too
dumb to live, but somebody who shall remain nameless but used to
maintain cdrecord seems to ahve hijacked the posix committee into
doing stupid and I haven't got time to try to engage with the stupid
and move an elephant out of my way. So, lib.c function qstrcmp() and I
walk away from the keyboard for a bit.
enh
2015-05-02 19:47:52 UTC
Permalink
Post by Rob Landley
Post by enh
Post by Rob Landley
I intend to finish and promote it this month, for what that's worth.
(I've got another 2 weeks...)
yeah, for now i've taken ps out of the Android .config. we didn't have
a symbolic link to it anyway, and although it's making progress, it's
not currently very likely that anyone would want to "toybox ps".
here's the current list of things from pending that get built into the
AOSP toybox binary. ones marked with * get a symbolic link too.
toys/pending/dd.c
toys/pending/expr.c *
toys/pending/hwclock.c *
toys/pending/more.c *
toys/pending/pgrep.c *
toys/pending/netstat.c *
toys/pending/route.c *
toys/pending/tar.c *
toys/pending/top.c
toys/pending/tr.c *
toys/pending/traceroute.c
those with links are either demonstrably better than what we had, or
things we just didn't have before. so although rob worries, it's not
like we're shipping all of pending and relying on it :-)
I'm cleaning up as fast as I can!
Speaking of which, here's a proposed getprop.c cleanup that I haven't
got a test environment for. The attached patch is infrastructure
changes, then the changed getprop.c file, and here's the notes from
---------
Proposed changes! (I'm not checking this in because I need your review
before doing that, I don't have your build environment or domain
expertise here.)
Instead of an #ifdef, let's add a "depends on TOYBOX_ON_ANDROID" so
you can only select the sucker when we're building on an android
system. (Meaning I can't test this, but I couldn't anyway because I
didn't have the header. I need to chop a build environment out of the
AOSP build...) [todo: teach "make change" about probed symbols.]
this breaks things for me. my goal is to have one .config file but
build for both the host and Android, so i can run tests on the host
(where i have stuff like awk).
Post by Rob Landley
Globals go in GLOBALS(), this includes "properties" here.
Google says PROPERTY_VALUE_MAX is 92, sizeof(toybuf) is 4096, so let's
just use that.
Since properties is an array of pointers to 2 entry structures, just
put the structs inline in the array. Since the name is the first entry
and you can typecast a struct pointer to a pointer to its first entry
(C99 says so), we can use strcmp() directly in qsort, so that function
goes away.
except results are no longer sorted.

Fix getprop sorting.

Use qstrcmp instead of alphasort (which expects struct dirent arguments).

diff --git a/toys/android/getprop.c b/toys/android/getprop.c
index 400d80e..71289f0 100644
--- a/toys/android/getprop.c
+++ b/toys/android/getprop.c
@@ -41,7 +41,7 @@ void getprop_main(void)
size_t i;

if (property_list((void *)add_property, 0)) perror_exit("property_list");
- qsort(TT.nv, TT.size, 2*sizeof(char *), alphasort);
+ qsort(TT.nv, TT.size, 2*sizeof(char *), qstrcmp);
for (i = 0; i<TT.size; i++) printf("[%s]: [%s]\n",
TT.nv[i*2],TT.nv[1+i*2]);
if (CFG_TOYBOX_FREE) free(TT.nv);
}
Post by Rob Landley
(Except we have to wrap it anyway to do an extra dereference. [See
GIANT_RANTY_FOOTNOTE at the bottom] So I added a qstrcmp() function to
lib/lib.c.)
Also the free() function is just free() the array at the end so that function
goes away.
Also in add_property we realloc() with 32 more slots each time size is
a multiple of 32, so we don't need a separate capacity counter. (Since 0
is an even multiple of 32 we alloc the initial data the same way.)
I'm assuming that property_list() will set errno on a failure, and thus used
perror_exit(). (This is a questionable assumption I can't test, hence the shout
out here.)
no, it doesn't. since you won't have applied the previous patch by the
time you get here...


Fix getprop sorting and error reporting.

Use qstrcmp instead of alphasort (which expects struct dirent arguments).

Don't use perror_exit because property_list doesn't set errno.

diff --git a/toys/android/getprop.c b/toys/android/getprop.c
index 400d80e..09bb0f0 100644
--- a/toys/android/getprop.c
+++ b/toys/android/getprop.c
@@ -40,8 +40,8 @@ void getprop_main(void)
} else {
size_t i;

- if (property_list((void *)add_property, 0)) perror_exit("property_list");
- qsort(TT.nv, TT.size, 2*sizeof(char *), alphasort);
+ if (property_list((void *)add_property, 0)) error_exit("property_list");
+ qsort(TT.nv, TT.size, 2*sizeof(char *), qstrcmp);
for (i = 0; i<TT.size; i++) printf("[%s]: [%s]\n",
TT.nv[i*2],TT.nv[1+i*2]);
if (CFG_TOYBOX_FREE) free(TT.nv);
}
Post by Rob Landley
About changing the xstrdup() prototype: Toybox doesn't use "const" because
I've never encountered a compiler that does anything intelligent with it
(what's it gonna do, put stack variables in a read-only segment?) and we
otherwise spend a huge amount of time getting it all to line up with no
http://landley.net/toybox/cleanup.html#advice
it helps catch careless mistakes. plus, here, it's weird to have a
wrapper around a function that takes const char* where the wrapper
takes the more restrictive (and misleading) char*.
Post by Rob Landley
(I'm aware property_list() wants const in its prototype, and am hitting that
with (void *) which is ugly but still fewer typecasts than before.)
but you'd have zero typecasts if xstrdup didn't have an incorrect prototype!
Post by Rob Landley
And looking at it: we don't really need the struct at all because it's just
pairs of char * with alternating meanings, that's simple enough to just do
inline. (This is a "six of one, half dozen of another" thing.)
Query: why size_t here? Is more than 2 billion of them likely to come up?
(Left it alone, but...)
size_t is the type of an index/size. if you use something _other_ than
size_t, your reader has to stop and wonder whether something subtle is
going on. if you use size_t, no thinking required.
Post by Rob Landley
Rob
qsort() really should have standard sort functions for things like
strcmp() on an array of pointers (or structs whose first member is a
pointer, which work the same way as far as qsort() is concerned). The
man page even has EXAMPLE CODE showing how to do this, and yet it's
not in libc and everybody needs to do their own gratuitous wrapper
function.
It _does_ say we can use alphasort(), which is wrong. That says it
wraps strcoll() which is equivalent to strcmp() in the "C" locale
(which we're in if we didn't feed TOYFLAG_LOCALE to the flags up top,
but I dunno if bionic actually implements that, even though it's in
C99. But that's not the wrong part, the wrong part is that when I
actually _try_ it I get incompatible pointer type because the man page
says it's alphasort(const void *a, const void *b) and the compiler is
saying "oh no, those arguments are struct dirent **"...
http://austingroupbugs.net/view.php?id=142
So posix-2008 is forcing people to use a typecast to get historical
behavior _in_the_name_of_type_safety_. That's just hilarious... (Ok,
looking deeper the Solaris guys broke this, then Jorg Schilling pushed
it upstream into posix, and it's up to _me_ to email Michael Kerrisk
to fix the man page _7_years_ later. And no, the solaris guys _really_
broke this so it doesn't work anymore even if you typecast. So the man
page needs fixing and the posix guys need somebody to mail them a box
of live stinging insects with a card that plays "seasons in the sun"
when you open it and doesn't stop when you close it again until the
battery runs down. Use a lithium battery.)
(anyone who cares has already moved on to C++ anyway. what the POSIX
folks do to qsort matters little to the people of 2015 passing lambdas
to std::sort or just using a sorted container. or, you know, Java or
Python.)
Post by Rob Landley
Anyway, I added a lib.c function for this. I was TEMPTED to add a
portability.c function and just declare posix's actions on this too
dumb to live, but somebody who shall remain nameless but used to
maintain cdrecord seems to ahve hijacked the posix committee into
doing stupid and I haven't got time to try to engage with the stupid
and move an elephant out of my way. So, lib.c function qstrcmp() and I
walk away from the keyboard for a bit.
you should have waited until you were using your new function before
walking away :-)
--
Elliott Hughes - http://who/enh - http://jessies.org/~enh/
Android native code/tools questions? Mail me/drop by/add me as a reviewer.
Rob Landley
2015-05-05 15:53:42 UTC
Permalink
Post by enh
Post by Rob Landley
Instead of an #ifdef, let's add a "depends on TOYBOX_ON_ANDROID" so
you can only select the sucker when we're building on an android
system. (Meaning I can't test this, but I couldn't anyway because I
didn't have the header. I need to chop a build environment out of the
AOSP build...) [todo: teach "make change" about probed symbols.]
this breaks things for me. my goal is to have one .config file but
build for both the host and Android, so i can run tests on the host
(where i have stuff like awk).
If you're building against an x86 version of bionic on the host, it'll
consider it android. If you _don't_ have that, it's a build break to
enable this stuff anyway...?
Post by enh
if (property_list((void *)add_property, 0)) perror_exit("property_list");
- qsort(TT.nv, TT.size, 2*sizeof(char *), alphasort);
+ qsort(TT.nv, TT.size, 2*sizeof(char *), qstrcmp);
for (i = 0; i<TT.size; i++) printf("[%s]: [%s]\n",
TT.nv[i*2],TT.nv[1+i*2]);
if (CFG_TOYBOX_FREE) free(TT.nv);
}
Oops. Implemented a fix, checked in the library file, did not check in
the change to the file that triggered it. (Sigh. The git status/diff
stuff vs mercurial status/diff stuff are just different enough to
repeatedly throw me off here...)
Post by enh
Post by Rob Landley
I'm assuming that property_list() will set errno on a failure, and thus used
perror_exit(). (This is a questionable assumption I can't test, hence the shout
out here.)
no, it doesn't. since you won't have applied the previous patch by the
time you get here...
Fix getprop sorting and error reporting.
Applied.
Post by enh
Post by Rob Landley
About changing the xstrdup() prototype: Toybox doesn't use "const" because
I've never encountered a compiler that does anything intelligent with it
(what's it gonna do, put stack variables in a read-only segment?) and we
otherwise spend a huge amount of time getting it all to line up with no
http://landley.net/toybox/cleanup.html#advice
it helps catch careless mistakes.
String constants being in the read-only data segment should help catch
careless mistakes. I've yet to see const do that. (It _can't_ for stack
data.)

plus, here, it's weird to have a
Post by enh
wrapper around a function that takes const char* where the wrapper
takes the more restrictive (and misleading) char*.
It's from me trying to be consistent within toybox, where nothing uses
const.

I broke down and put the const on qstrcmp() because every single use of
it would have to be typecast otherwise. (I've been tempted to switch the
dlist stuff to void * for the same reason.) But xstrcmp() shouldn't
throw warnings whether or not you feed it a const?
Post by enh
Post by Rob Landley
(I'm aware property_list() wants const in its prototype, and am hitting that
with (void *) which is ugly but still fewer typecasts than before.)
but you'd have zero typecasts if xstrdup didn't have an incorrect prototype!
Or if property_list() didn't have a meaningless qualifier that exists
solely to be matched with other meaningless qualifiers.

(I experimented with "#define const" as the first include early in
toybox' history. It didn't help. I forget why.)
Post by enh
Post by Rob Landley
And looking at it: we don't really need the struct at all because it's just
pairs of char * with alternating meanings, that's simple enough to just do
inline. (This is a "six of one, half dozen of another" thing.)
Query: why size_t here? Is more than 2 billion of them likely to come up?
(Left it alone, but...)
size_t is the type of an index/size.
But that logic the i in for loops should be size_t.
Post by enh
if you use something _other_ than
size_t, your reader has to stop and wonder whether something subtle is
going on. if you use size_t, no thinking required.
For me it was the other way. I had to stop and think about whether there
was something subtle going on.

I like to know what types I'm actually using. This is why toys.optflags
is an int rather than a long: on 32 bit platforms you get 32 bits (thus
32 possible flags). On 64 bit platforms, you get: 32 bits. You don't
have something that works on 64 bits but _doesn't_ work on 32 bits.

But size_t varies with "long file support" (archaic but still possible
to forget to enable because the glibc default is still crazy:
https://sourceware.org/ml/libc-alpha/2014-03/msg00092.html ). This is
the _reason_ for size_t. Because it might _not_ be 64 bits.

The above "i in for loops" example has a built-in reason _not_ to use
it: manipulating 64 bit numbers on 32 bit systems is awkward so you
_don't_ want to have a 64 bit index counter in a performance critical
inner loop in that context (and you _really_ want to avoid 64 bit
divides there). That isn't the case here but also isn't something you
want to forget and do it wrong where it does matter. (I've seen people
complain that large file support slowed down their programs and it was
generally them pulling that sort of thing.)

This isn't a big enough deal for me to bother to change it, but the "no
thinking required" above? Not a compelling argument for me in this
instance. :)
Post by enh
Post by Rob Landley
Rob
...
Post by enh
(anyone who cares has already moved on to C++ anyway. what the POSIX
folks do to qsort matters little to the people of 2015 passing lambdas
to std::sort or just using a sorted container. or, you know, Java or
Python.)
And when they collapse under the weight of their endlessly increasing
complexity, C will presumably still be here. :)

(The saving grace of ISO-WG14 and the Austin group is neither actually
does very much, and when they do they're mostly* documenting existing
practice. I'm still ignoring the 2011 C standard, because I can. I read
the summaries when it came out, and other than the new thread
synchronization stuff (NNPTL? NITL?) I think it was all fluff. The musl
guys do care, and I cheer them on from the sidelines with popcorn.)

(Don't get me started on Python. Honestly, python 3 is not the same
language as python 2. One obvious way to do it indeed. They have
_more_than_one_ conference panel at pycon about the lack of uptake of
their rewrite (ala https://lwn.net/Articles/640181/ and
https://lwn.net/Articles/640179/) six and a half years after its
release, and they don't think this is damaging the language? A c89
program will generally compile and run under a c99 compiler. Python 3
breaks python 2. This is a problem.)

* Yeah, I know.
Post by enh
Post by Rob Landley
Anyway, I added a lib.c function for this. I was TEMPTED to add a
portability.c function and just declare posix's actions on this too
dumb to live, but somebody who shall remain nameless but used to
maintain cdrecord seems to ahve hijacked the posix committee into
doing stupid and I haven't got time to try to engage with the stupid
and move an elephant out of my way. So, lib.c function qstrcmp() and I
walk away from the keyboard for a bit.
you should have waited until you were using your new function before
walking away :-)
I totally should have, yes. Should be fixed now...

Rob
enh
2015-05-05 16:00:50 UTC
Permalink
Post by Rob Landley
Post by enh
Post by Rob Landley
Instead of an #ifdef, let's add a "depends on TOYBOX_ON_ANDROID" so
you can only select the sucker when we're building on an android
system. (Meaning I can't test this, but I couldn't anyway because I
didn't have the header. I need to chop a build environment out of the
AOSP build...) [todo: teach "make change" about probed symbols.]
this breaks things for me. my goal is to have one .config file but
build for both the host and Android, so i can run tests on the host
(where i have stuff like awk).
If you're building against an x86 version of bionic on the host, it'll
consider it android. If you _don't_ have that, it's a build break to
enable this stuff anyway...?
no, i'm building against glibc with your "make" because (a) it's a useful
way to double-check behavior and (b) i'm relying on the generated/ files
created by that. for now i've put the #if defined(__ANDROID__)s back.

i do want to move to where i'm generating the generated files at
build-time, but i need to be able to tell all that stuff where "generated/"
is (it needs to be in a directory of the build system's choosing, in the
"out" tree), and i need to supply the mkflags that the build system has
already built.

even when that's done, i'll still have a checked-in .config file, and i
won't want to have to maintain a .config.glibc that should be identical
except for toys that include <cutils/properties.h>.
Post by Rob Landley
Post by enh
if (property_list((void *)add_property, 0))
perror_exit("property_list");
Post by enh
- qsort(TT.nv, TT.size, 2*sizeof(char *), alphasort);
+ qsort(TT.nv, TT.size, 2*sizeof(char *), qstrcmp);
for (i = 0; i<TT.size; i++) printf("[%s]: [%s]\n",
TT.nv[i*2],TT.nv[1+i*2]);
if (CFG_TOYBOX_FREE) free(TT.nv);
}
Oops. Implemented a fix, checked in the library file, did not check in
the change to the file that triggered it. (Sigh. The git status/diff
stuff vs mercurial status/diff stuff are just different enough to
repeatedly throw me off here...)
Post by enh
Post by Rob Landley
I'm assuming that property_list() will set errno on a failure, and thus
used
Post by enh
Post by Rob Landley
perror_exit(). (This is a questionable assumption I can't test, hence
the shout
Post by enh
Post by Rob Landley
out here.)
no, it doesn't. since you won't have applied the previous patch by the
time you get here...
Fix getprop sorting and error reporting.
Applied.
Post by enh
Post by Rob Landley
About changing the xstrdup() prototype: Toybox doesn't use "const"
because
Post by enh
Post by Rob Landley
I've never encountered a compiler that does anything intelligent with it
(what's it gonna do, put stack variables in a read-only segment?) and we
otherwise spend a huge amount of time getting it all to line up with no
http://landley.net/toybox/cleanup.html#advice
it helps catch careless mistakes.
String constants being in the read-only data segment should help catch
careless mistakes. I've yet to see const do that. (It _can't_ for stack
data.)
plus, here, it's weird to have a
Post by enh
wrapper around a function that takes const char* where the wrapper
takes the more restrictive (and misleading) char*.
It's from me trying to be consistent within toybox, where nothing uses
const.
I broke down and put the const on qstrcmp() because every single use of
it would have to be typecast otherwise. (I've been tempted to switch the
dlist stuff to void * for the same reason.) But xstrcmp() shouldn't
throw warnings whether or not you feed it a const?
Post by enh
Post by Rob Landley
(I'm aware property_list() wants const in its prototype, and am hitting
that
Post by enh
Post by Rob Landley
with (void *) which is ugly but still fewer typecasts than before.)
but you'd have zero typecasts if xstrdup didn't have an incorrect
prototype!
Or if property_list() didn't have a meaningless qualifier that exists
solely to be matched with other meaningless qualifiers.
(I experimented with "#define const" as the first include early in
toybox' history. It didn't help. I forget why.)
Post by enh
Post by Rob Landley
And looking at it: we don't really need the struct at all because it's
just
Post by enh
Post by Rob Landley
pairs of char * with alternating meanings, that's simple enough to just
do
Post by enh
Post by Rob Landley
inline. (This is a "six of one, half dozen of another" thing.)
Query: why size_t here? Is more than 2 billion of them likely to come
up?
Post by enh
Post by Rob Landley
(Left it alone, but...)
size_t is the type of an index/size.
But that logic the i in for loops should be size_t.
Post by enh
if you use something _other_ than
size_t, your reader has to stop and wonder whether something subtle is
going on. if you use size_t, no thinking required.
For me it was the other way. I had to stop and think about whether there
was something subtle going on.
I like to know what types I'm actually using. This is why toys.optflags
is an int rather than a long: on 32 bit platforms you get 32 bits (thus
32 possible flags). On 64 bit platforms, you get: 32 bits. You don't
have something that works on 64 bits but _doesn't_ work on 32 bits.
But size_t varies with "long file support" (archaic but still possible
https://sourceware.org/ml/libc-alpha/2014-03/msg00092.html ). This is
the _reason_ for size_t. Because it might _not_ be 64 bits.
The above "i in for loops" example has a built-in reason _not_ to use
it: manipulating 64 bit numbers on 32 bit systems is awkward so you
_don't_ want to have a 64 bit index counter in a performance critical
inner loop in that context (and you _really_ want to avoid 64 bit
divides there). That isn't the case here but also isn't something you
want to forget and do it wrong where it does matter. (I've seen people
complain that large file support slowed down their programs and it was
generally them pulling that sort of thing.)
This isn't a big enough deal for me to bother to change it, but the "no
thinking required" above? Not a compelling argument for me in this
instance. :)
Post by enh
Post by Rob Landley
Rob
...
Post by enh
(anyone who cares has already moved on to C++ anyway. what the POSIX
folks do to qsort matters little to the people of 2015 passing lambdas
to std::sort or just using a sorted container. or, you know, Java or
Python.)
And when they collapse under the weight of their endlessly increasing
complexity, C will presumably still be here. :)
(The saving grace of ISO-WG14 and the Austin group is neither actually
does very much, and when they do they're mostly* documenting existing
practice. I'm still ignoring the 2011 C standard, because I can. I read
the summaries when it came out, and other than the new thread
synchronization stuff (NNPTL? NITL?) I think it was all fluff. The musl
guys do care, and I cheer them on from the sidelines with popcorn.)
(Don't get me started on Python. Honestly, python 3 is not the same
language as python 2. One obvious way to do it indeed. They have
_more_than_one_ conference panel at pycon about the lack of uptake of
their rewrite (ala https://lwn.net/Articles/640181/ and
https://lwn.net/Articles/640179/) six and a half years after its
release, and they don't think this is damaging the language? A c89
program will generally compile and run under a c99 compiler. Python 3
breaks python 2. This is a problem.)
* Yeah, I know.
Post by enh
Post by Rob Landley
Anyway, I added a lib.c function for this. I was TEMPTED to add a
portability.c function and just declare posix's actions on this too
dumb to live, but somebody who shall remain nameless but used to
maintain cdrecord seems to ahve hijacked the posix committee into
doing stupid and I haven't got time to try to engage with the stupid
and move an elephant out of my way. So, lib.c function qstrcmp() and I
walk away from the keyboard for a bit.
you should have waited until you were using your new function before
walking away :-)
I totally should have, yes. Should be fixed now...
Rob
--
Elliott Hughes - http://who/enh - http://jessies.org/~enh/
Android native code/tools questions? Mail me/drop by/add me as a reviewer.
Continue reading on narkive:
Loading...