More bbsh design notes.

[Rob Landley]

On Thursday 14 September 2006 12:16 pm, Bernhard Fischer wrote:
> Random comments while reading the currently visible incarnation..

I didn't ask.  You're critiquing a random snapshot of code that's NOT 
FINISHED.

That said, since you went to all the trouble of looking at it, I'll reply...

> run_pipeline() currently is only used by handle() and should be merged
> into it IFF it stays to be the only user.

A single job can have multiple pipelines (blah | blah && blah | blah) so this 
is going to get called from a loop.  It's _way_ easier on me if I can keep 
the guts of run_pipeline separate from the caller right now.

> run_pipeline(): save 9 bytes by reusing skip_whitespace().

Premature optimization is the root of all evil - Tony Hoare

I'll pay attention to that sort of thing _later_, right now I've moved the 
check for ; and # and stuff into and out of that loop, and it's still 
potentially moving around.

> Why are you doubling code for list management? Use llist_free() instead
> of free_list()?

Because those lists require two allocations (a struct for the list itself and 
then a pointer to the object the list contains), and the lists I'm working 
with right now have a pointer as the first element of the list but then the 
rest of that struct is the contents.

Yes, it's intentional, and I'm pondering what would be involved in migrating 
the llist functions to do things that way, but later...

> run_pipeline(): missing newline in print("NO %s");

I already fixed that days ago in my copy.

> Why don't you just 
> use bb_perror_msg_and_die? Doing so saved another 8 byte.

Because I want to exit with 127 rather than 1 (it's in the spec), and that 
takes back a few bytes.  Plus this is the only occurrence of 
perror_msg_and_die because in pretty much every other case a shell shouldn't 
_exit_, it should continue running on errors.  So sucking this in on a 
standalone build is a net loss.  It's something I plan to look at later (if 
it occurs twice in this applet, then it's probably worth it, and the builtin 
functions will probably use it).

But again, I have reasons for not doing this: yet.

> run_pipeline(): There is still that pointless status integer. Ditched to
> save 4b

Which pointless status integer?  The return value?  Eventually && and || will 
have to return whether or not to run the next chunk, and the upstream for 
loop will care, and there's no way I'm making the return type of the function 
vary based on a CONFIG option.  (I don't _care_ if it saves 4 bytes, I'm not 
doing the #ifdef or some uber-funky typedef macro.)

> run_pipeline(): cache argv0 to save another 5b

It's very likely the built-in functions will get moved out to separate 
functions with a table similar to lash's.  With the code the way it is now, 
that's very easy to do.  If I decide not to do that, I can do local cleanups 
later.

> pipeline.job_id commented out; Currently not used.

A number of things currently aren't used, yes.  I would have thought that was 
obvious.  It's not shipping like this.

> command.flags perhaps unsigned?

*shrug* when I need the bit. :)

> Quick impl of `export´ and `unset´..

I have a config option for those.  They're useless until the shell can parse 
and substitute $variables into the command line arguments, which is why I 
haven't bothered to do them yet.

Also note that there are two types of shell variables; local and environment.   
And yes, unset needs to know about both kinds...

> Wants a table for these builtins and chdir et al.

I mentioned that, yes.  It's possible that the if/else staircase would be 
smaller, but the table would be more maintainable, and I haven't decided yet.  
I'm still writing it.

> Need to play with it some more.. later

The version in the tree is stale.  That statement is likely to remain true for 
a while to come. :)

It's not to the point where multiple people can really work on it yet.  I've 
been designing and prototyping disconnected bits for months and am gluing 
them on carefully one at a time with a lot of "but no wait, then this part 
has to do _this_..."

I'm really not thinking about microoptimizations right now.  They only get in 
the way.  I am thinking "and this config option barrier can bump up 2 lines 
if I do things in this order", but that's at a pseudo-code level at best.

> cheers,
> Bernhard

Rob
-- 
Never bet against the cheap plastic solution.