Chat applet growing

[Bernhard Fischer]

On Tue, Feb 12, 2008 at 07:11:23PM +0300, Vladimir Dronnikov wrote:
>>
>> >static int timeout = DEFAULT_CHAT_TIMEOUT;
>>
>> unsigned?
>
>
>I'd prefer signed as I might specify TIMEOUT -1 to switch timeout off. This
>requires change in timeout validation below.

Well, a timeout of 0 could mean that it's off, no?
>
>
>> >static void signal_handler(int signo)
>>
>> ATTRIBUTE_UNUSED missing for signo
>
>
>Right! Missed. Earlier I used signo...
>
>>static int chat_expect(char *answer, bool echo)
>>
>> static small{u,}int ?
>
>
>Hmm.  +3 byte. No.

ok.
>
>>
>> >       //bb_error_msg("EXPECT: [%s]", answer);
>>
>> Please remove this debugging code. And you most likely mean bb_info_msg.
>
>
>No. chat can't use stdout.

ah, ok.
>Comments will be removed when chat becomes alpha, ok?

Well, i'm all for ripping cruft out earlier than later, but if you need
it for debugging, then i'd have added a DBG(...) macro and use that, but
ok :)
>
>>               // read next char(s) from device
>> >               if (safe_read(STDIN_FILENO, buf, 1) > 0) {
>>
>> Why are you just reading one char at once?
>
>
>Because I got to analyse accumulated answer as it comes.
>As soon as I find a match I must stop reading and return to sending mode.
>It's the chat feature.

aha, ok then.
>
>> >                               if (ENABLE_FEATURE_CHAT_BLOATY)
>>
>> CHAT_BLOATY is misleading, this is more a CHAT_{VERBOSE,DEBUG}, no?
>
>
>In fact I just want to ask: do we need this verbosity and debug?
>I'm inclined to just throw away all the content between *_BLOATY when it
>comes to alpha.

Yes, just rip it out *now* :)
>
>>                                       bb_error_msg("LINE TOO LONG");
>> >                               return ERR_MEM;
>> >                       }
>> >               }
>> >               // abort matched?
>> >               for (l = aborts, i = ERR_ABORT; l; l = l->link, ++i) {
>>
>> Well, you would not have needed a global "aborts" if you would have
>> inlined this function in the first place.
>
>
>Again, let me first debug the logic. Optimizations will come then, ok?

hmz. The logic should be simple and concise, automagically resulting in
acceptable size :) You (usually) cannot have one without the other.

>>                       if (strstr(bb_common_bufsiz1, l->data)) {
>> >                               if (ENABLE_FEATURE_CHAT_BLOATY)
>> >                                       bb_error_msg("ABORTED: [%s]",
>> l->data);
>> >                               return i;
>> >                       }
>> >               }
>>
>> Is it smaller to
>> llist_t *abort_p = aborts;
>> while (aborts_p) {
>> if (strcmp()==0)
>>         ;
>> aborts_p = aborts_p->link;
>> }
>
>
>To be checked. Note the use of strstr() not strcmp(). In fact I miss
>index_in_* function which calls strstr().

index_in_substr_array
IIRC strstr is quite bloaty, avoid it if possible.

>
>> >
>> >       return ERR_TIMEOUT;
>> >}
>> >
>> >static int chat_send(char *s)
>>
>> That, too should be inlined manually, no need to introduce a separate
>> function for it..
>
>
>When I started brctl you showed how to reduce code. I thought I wrote
>compact code :^)
>>From then I constantly try to code in several ways.
>When I played with microcom I noticed that to use a function sometimes is
>better and produces less bloat.
>GCC is not human :)
>Still I'd prefer to save functions because of upcoming functionality which
>won't be reduced to inline code.
>I mean so-called subexpect-subexpect pairs in chat, when chat_expect() might
>have to call chat_send().

You should start off with inlined code (or use already existing
infrastructure) and only if you desparately need it (i.e. if you cannot
come up with a logic where you can fallthrough to the inline-code from
several spots or if falling through is not benefical anymore since the
optimizers don't grok what you mean) you should factor it out to a
function, really.

Don't add debugging code (e.g. CHAT_BLOATY) and don't prematurely factor
stuff out for no immediate benefit :)

>>{
>> >       struct pollfd pfd;
>> >
>> >       char *data = NULL; // loaded command
>> >       int cr = 1; // implicitly append CR to command
>>
>> bool or small{u,}int if you cannot get rid of it entirely.
>
>
>+2 bytes in both cases. No.
>Stack is stack. From bool to int anything is 4 bytes long. But if we check a
>byte we need more assembly code.
>GCC...

I figure that it would help for the inline-case, depending on the
liveness of other variables, but ok.
>
>>
>> >//     bb_error_msg("SEND: [%s]", s);
>> >
>> >       // if command starts with @
>> >       // load "real" command from file named after @
>> >       if ('@' == s[0]) {
>>
>> *s any different?
>
>
>I don't know just who is right! One tells to use s[0], another -- *s.
>I myself prefer *s. So what is BB style?

Array-refs used to be bigger, ISTR.
I don't think there is a strict style.
>
>> >       // send command
>> >       pfd.fd = STDOUT_FILENO;
>> >       pfd.events = POLLOUT;
>> >       while (*s && safe_poll(&pfd, 1, timeout) > 0) {
>> >               char c;
>> >               // POLLHUP causes break
>> >               if (!(pfd.revents & POLLOUT))
>> >                       break;
>> >               // do we need special processing?
>>
>> The if block below looks like it's a bit suboptimal. Can you rephrase
>> this to improve it, size-wise?
>
>
>Of course. But again, it can give weird results :)

heh. Of course without producing wrong results ;)
>
>> Please use an enum for the keys and the words, it's easier to read and
>> change.
>
>
>Hmm. enum-ed in the top of main().

thanks.
>
>Also, please include size(1) for new applets.
>> TIA,
>>
>
>Right now chat.o gives 1130 bytes using standard BB objsize(s).

Ok, so let's aim at getting it below 1000 Bytes before sending a final
draft.
>
>Thank you a lot for your comments!

thanks for your work on chat