[Toybox] [landley/toybox] Simple implementation of less (#39)

[Rob Landley]

On 07/26/2016 02:22 AM, Toni Spets wrote:
> @hifi <https://github.com/hifi> pushed 1 commit.
> 
>   * 79ca172 <https://github.com/landley/toybox/commit/79ca172> Less
>     magic, more printf

It is so weird that github shows that commit as living in my tree
instead of yours. I'm glad I only use that site for distribution of
masters I keep elsewhere. (Is every commit in the whole of github in a
single giant tree with a flat sha1hash collision namespace? I wonder how
git catches that, or if the don't care until
http://valerieaurora.org/hash.html goes red?)

Ok, looking at git diff 7918d9ff8..79ca172eb0d0

The patch adds scr_init() allocating a screen buffer with 4 bytes per
character... not an array of wide characters, but a char * with a width
you multiply by 4. Ok? I have no idea how you're going to fit combining
characters into there. You also don't appear to be storing color
information, which is something less -R outputs...

(So when you scroll the screen back up, you're not just outputting ESC[T
and writing a new line of data at the top, but instead doing a memmove
on your internal screen buffer? Why?)

Next function: the patch adds scr_linediff()... which has no comment
explaining what it does. It's doing a memcmp on an an array of char
pointers... scr_init() allocated exacty 2 char pointers, where does this
array with a length come from...

Ok, let's jump down to the last function added to this file and see if
it calls the earlier ones... scr_printf() is printing into SCR_PTR().
What's SCR_PTR? It's added to lib.h as

  #define SCR_PTR(scr) (scr->buf[0])

And this is the only user of that macro anywhere in the code so why is
it a macro?

If you try to write to >x or >y it returns with no error. Otherwise,
this function is a wrapper around:

  vsnprintf(SCR_PTR(scr) + (scr->l * y), scr->l - x, fmt, va);

That finds the start of the y line (scr->l is scr->w*4 and I don't know
why you needed to save that to avoid a <<2 in the users), does _not_
advance x bytes (or bytes*4) into it, but truncates what we're printing
to what presumably _would_ be the end of the line if we had advanced.

Again, our screen width is *4 but we're not printing wide characters
here. (Did you mean vswprintf?)

ah, the second buffer _isn't_ to store color information, it's so you
can diff what you think is on the screen (before the xterm was resized?)
with what is now on the screen.

So, next function up: scr_update() and THAT calls scr_linediff.
Iterating through scr->h. What is scr->h? In lib.h it's "unsigned".
Great. Back to look at scr_init()... Ah, it's screen height. Ok...

+    char *line = scr_linediff(scr->buf[1] + (scr->l * i),
+                              scr->buf[0] + (scr->l * i),
+                              scr->l,
+                              i + 1);

This is the only user of scr_linediff() in the code. Why it's a
function, I couldn't tell you.

Ah, I see! Those for() loops at the start of scr_linediff() are
open-coding strnlen(). A non-wide-character version, which will stop at
the first zero byte (which wide characters tend to be full of).

And that's the chunk of time I had, I have a conference call in 12
minutes so need to go prepare for that...

Rob