On 04/10/2013 01:55:58 PM, Tim Bird wrote:
> - dump_node is not used anywhere
It used to be used in debug code that was recently removed
by Rob. It's a bit of a pain to examine the expression
tree in a debugger,
Simplifying the expression tree is something I need to look at.
so an ascii dump routine is handy.
Because it is unused, it evaporates because of compiler
optimizations. I don't know if Rob left it around intentionally
or not, but I hate to see it go while 'find' is still under
development. (Once we're all happy with the find implementation,
of course, you can feel free to remove the lifeboats, just like
the Titanic. :-)
In general, I don't know if Rob has a policy for having debug
code available for use, but not currently used.
I try not to _need_ it.
I admit that there's a bit of debug scaffolding in patch.c and
lib/args.c, but that's a sign of failure on my part. I periodically go
back and look at this to see if I can think of a way to simplify that
code. (Both were hard enough for me to implement that there's still
some scar tissue, and both have pending todo items giving me an excuse
to defer further cleanup until I revisit them. Patch needs to support
git mv syntax, for example...)
Sometimes I just have to say "I'm not smart enough to come up with less
horrible way to do this right now" and move on, but I periodically
revisit those to see if I get lucky.
> - exec_buf is unused
I think I put this there in anticipation of the work required
to properly support exec. Right now, the code doesn't
scan the args and replace '{}' with the file or directory
name being evaluated. I think I planned to use exec_buf
for this substitution, rather than allocate something on
every evaluation. But that's a detail that whoever finishes
the exec functionality can work out.
This is the sort of thing toybuf[] is there for: a roughly page sized
chunk of memory you don't have to allocate.
I've subdivided toybuf for various uses, such as ls.c function
listfiles() which is more opaque black magic than I like but it's doing
a fiddly thing in a compact way that's easy to read is _hard_.
Then again arbitrary size limitations aren't good. I need to clean
those out of toysh when I restart work on that. It's only acceptable in
ls because the things I'm using it for have existing limits anyway: the
maximum filename length is limited to 255 in the vfs (rounded up to
260, a multiple of 4), and then the remainder of the 4k, divided by
sizeof(long), is going to be at least 500, which would be a screen
width of at _least_ 1000 characters, and that's just uncomfortable for
humans (the norm is still in the ballpark of 80, that's an order of
magnitude more, you can't figure out what line you're on when you jump
back to the start when it's anywhere near that big)... so the fixed
size buffer is overkill in that case. :)
> - Replace SUCCESS with 1 and simplify code accordingly
> - a==0 -> !a
I'm OK with most of these, when 0 is being treated as
failure and !0 as success (all the string operations).
That's how tests work in C. :)
There is at least one place where I'm actually checking
that something is equal to the value 0 (that is, it's
a value comparison and not a boolean). I know that's a
bit picky, and they compile the same, but mentally I
prefer to use numbers for number comparisons and ! for
boolan not.
*shrug*
> -char exec_buf[1024];
> -
See note above. It might have been intended for arg substitution
for -exec.
Both times you use toybuf it's to store a copy of a string you have a
constant version of. (Environment space never gets freed, and "." is a
string constant.) so you could just have a pointer in your GLOBALS() or
some such that gets initialized or is otherwise NULL.
[wanders away to go fix that... ok back now.]
> - printf("unknown node type (%d) ", node->op);
> - break;
> - }
> -}
> -
See above. It's going to be harder to debug filter expressions
without the ability to dump the filter nodes in ascii.
The fix to that is to simplify filter expressions. Working up to that...
> pid = fork();
> - if (pid==0) {
> + if (!pid) {
I prefer pid==0 here. !pid has the same effect in the compiler,
but I'm not doing a boolean compare, I'm checking for the special
value
0 from fork. IMHO it's easier to read with the 0 present.
I lean towards "0 is 0" myself.
(And I redid this whole area in the most recent commit.)
> if (filter->op==OP_OR) {
> - result = evaluate(filter->next, node, fnext);
> - result2 = evaluate(*fnext, node, fnext);
> - if (result) {
> - return result;
> - } else {
> - if (result2) {
> - return result2;
> - } else {
> - return 0;
> - }
> - }
> + return evaluate(filter->next, node, fnext) ||
evaluate(*fnext, node, fnext);
This will have different side effects than the original code.
The original code evaluates both sides, and then returns the 'or'
operation
on the results. The new one will (of course) do short-circuit
evaluation of
the '||', meaning that sometimes the second node won't be evaluated.
I believe that at one time, I actually had it coded that way, but for
some
reason or other I went back to forcing the evaluation of both nodes.
Unfortunately, I can't remember the reason why I went back. I would
have expected
regular find to do short-circuit evaluation of expressions with 'or'
operations,
so I'd be happy if this were correct. I just have a nagging feeling
that
there was some reason I couldn't do it this way.
This is why I try to fill out the test suite as I go along. Because
otherwise I wind up in exactly this sort of situation...
Do you know if regular find does short-circuit evaluation?
What would regular find do with
'find . -exec test1_with_side_effects {} \; -o -exec
test2_with_side_effects {} \;'
?
$ find . -exec echo one "{}" \; -o exec echo two "{}" \;
find: paths must precede expression: exec
Um... huh?
> result = evaluate(filter_root, node, &junk);
> - if (result & SUCCESS & !have_action) {
> + if (result & !have_action) {
I have no explanation for my weird code. I think it was
left over from when SUCCESS was something besides 1, and I had
to do an actual value compare here. But the old code is
clearly just broken now. Thanks for the fix.
There's never a need to apologize for it. I have thinkos all the time
(3am sleep deprived coding, fun for the whole family). It's better than
not having it, and we're fixing it. :)
> @@ -570,7 +430,7 @@
>
> /* FIXTHIS - parse actions, if present */
>
> - if (toybuf[0]==0) {
> + if (!toybuf[0]) {
I prefer the comparison with 0 here. The 0 value
at the begging of the toybuf is not a boolean (in my mind anyway)
but a special value. But that's just me. It's all just
mental model, and if the '!' is easier to read for others
I'm fine with this change.
I do !*toybuf but that's just me. It's largely the same to the
compiler. :)
Rob
_______________________________________________
Toybox mailing list
[email protected]
http://lists.landley.net/listinfo.cgi/toybox-landley.net
