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