Re: lynx-dev disappearing empty lines (was: THANKS AND QUESTION)

[Vlad Harchev]

On Tue, 4 Jan 2000, Klaus Weide wrote:

> Actually, looking at print_wwwfile_to_fd in dev.17, the following
> logic for suppressing newlines is wrong:
> 
>     for (;; line = line->next) {
>         if (!first &&
>          line->data[0] != LY_SOFT_NEWLINE &&
>          line->data[0] != '\0' &&
>          line->data[1] != LY_SOFT_NEWLINE) {
>             /* data[0] can be LY_*START_CHAR, so LY_SOFT_NEWLINE can be in 
> [1] - VH */
>             fputc('\n',fp);
> 
> This will quite obviously give the wrong behavior (non-execution of
> hte fputc) in the case of line->data[0] == '\0' (a completely empty
> line, no special chars either).

  Oops, you are right.
 
> The line->data[0] != '\0' condition was added by J. Bley, to guard
> against accessing line->data[1] which may be invalid memory if the
> line size is 0.  The unguarded access to line->data[1] was added
> a bit earlier by VH (see comment).  VH didn't check JB's modification
> of his code.

  Yes, I noted this modification only today. Seems that this modification
didn't occur in changelog, and I don't read each line of the patches.
   
> A corrected logic for the condition would be
> 
>         if (!first &&
>          line->data[0] != LY_SOFT_NEWLINE &&
>          !(line->data[0] != '\0' &&
>            line->data[1] == LY_SOFT_NEWLINE)) {
> 
> which is the same as
> 
>         if (!first &&
>          line->data[0] != LY_SOFT_NEWLINE &&
>          (line->data[0] == '\0' ||
>           line->data[1] != LY_SOFT_NEWLINE)) {

  I prefer this version.  

> or
> 
>         if (!first &&
>         (line->data[0] == '\0' ||
>            (line->data[0] != LY_SOFT_NEWLINE &&
>             line->data[1] != LY_SOFT_NEWLINE))) {
> 
> Actually, to match more closely the motivation from the comment, replace
> line->data[0] != '\0' with IsSpecialAttrChar(line->data[0]).

     I like current variant - it's more verbose.

> The following function, print_crawl_to_fd, has the same original problem
> of potential invalid access to line->data[1], yet has not suffered through
> a wrong correction attempt.

  I will sent a patch against it too if it's needed (if Tom haven't fixed it 
already).
 
>   ---
> 
> While corrections are obvious, I still find the logic behind VH's change
> questionable:
> 
>             /* data[0] can be LY_*START_CHAR, so LY_SOFT_NEWLINE can be in 
> [1] - VH */
> 
> Why and since when?  Has this always been a problem (if yes, why was
> it not found earlier?), or only been introduced by, say, -prettysrc
> source mode?  Or one of the other new modes with which VH has blessed
> us more recently?
 
 Of course this case was made possible by the addition of psrcmode
and/or -dump-with-backspaces (since there was no modes in which 
LY_SOFT_NEWLINE was emitted in the text, that contained LY_*START_CHAR). 
-dump-with-backspaces mode and psrcmode make such case possible.
 
> If data[0], can be LY_*START_CHAR, then so can presumably data[1], and
> possibly more.   Without knowing that that can never actually happen,
> the added test looks like a partial ad hoc fix up, not a real solution.

 As I understand GridText.c and SGML.c, LY_BOLD_START_CHAR can't be nested.
 So LY_UNDERLINE_START_CHAR can't be nested too. I'm not sure whether
 LY_U*START_CHAR can immedeately follow LY_B*START_CHAR and vice versa (please
answer). So, there is a chance that data[2] also should be compared against
LY_SOFT_NEWLINE.
 
> IMO the code that adds LY_SOFT_NEWLINE to a line should make sure that
> it gets put at the beginning of a line, maybe by re-ordering before
> LY_*START_CHARs if necessary, so that no fixups are necessary in other
> code that later has to process the lines.

 I agree that this is a cleaner way, but I'm not sure whether it would be easy
to make swappings with CJK texts, etc (in other words, making these fixups is
much easier for me). If you think it's easy to do this swapping, could you 
please do the swapping yourself?
 Please answer whether you'll implement swapping.
 
 Tom, you said that you have fixed the bug too... Tom, should I produce a
patch?

> Vlad, please send patches.
> 
> 
>    Klaus
> 

 Best regards,
  -Vlad