Re: scratch/fontify-open-string. [Was: CC Mode and electric-pair "problem".]

[Alan Mackenzie <acm@muc.de>]

Hello, Eli,

thanks for the review.  The code is still preliminary, and is missing
quite a lot of comments, still.

I have had doubts about the mechanism (e.g. C-M-b will take a lot of
work to make it functional), see my reply to Stephen.

On Sun, Jul 15, 2018 at 18:13:15 +0300, Eli Zaretskii wrote:
> > From: Stephen Leake <stephen_leake@stephe-leake.org>
> > Date: Sun, 15 Jul 2018 04:00:23 -0500

> > Anything I can do to help merge this to main?

> A few things:

>  . NEWS
>  . Updates for the relevant parts in the manual(s)
>  . Minor nits below:

> > +(defcustom font-lock-warn-open-string t
> > +  "Fontify the opening quote of an unterminated string with warning face?
> > +This is done when this variable is non-nil.

> We use a slightly different style for such options (slightly rephrased
> to fit on one line):

Well done for the compression!

>   "Non-nil means show opening quotes of unterminated strings with warning face."

> > +This works only when the syntax-table entry for newline contains the flag `s'
> > +\(see page \"xxx\" in the Elisp manual)."

> Please replace "xxx" with an actual value.  Also, we don't refer to
> our manuals as "pages", that is a relic from the "man pages" era.

Yes, thanks.  Just "see \"<page name>\"", without the "page"?

> > +#define DEC_AT                                                  \

> Please #undef DEC_AT when you are done using it (at function's end).

OK.

> > +  /* Find the alleged string opener. */

> Please leave 2 spaces between the end of the comment and "*/" (here
> and elsewhere in the patch)

OK.  As a matter of interest, what is the reason for this?  I've seen
it all over the Emacs C code.  Is it something to do with filling?

> > +  while ((at > stop)
> > +         && (code != Sstring)
> > +         && (!SYNTAX_FLAGS_CLOSE_STRING (syntax)))
> > +    {
> > +      DEC_AT;
> > +    }

> A single line doesn't need braces.

I'm intending to put more code in there.

> > +      /* Search back for a terminating string delimiter: */
> > +      while ((at > stop)
> > +             && (code != Sstring)
> > +             && (code != Sstring_fence)
> > +             && (!SYNTAX_FLAGS_CLOSE_STRING (syntax)))
> > +        {
> > +          DEC_AT;
> > +          /* Check for comment and "other" strings. */
> > +        }

> Is the last comment at its correct place?  It doesn't seem to refer to
> any code.

It's a FIXME: "Put in code here to check for comment and "other"
strings.".

> > + lose:
> > +  UPDATE_SYNTAX_TABLE_FORWARD (*from);
> > +  return false;
> > +
> > + lossage:
> > +  /* We've encountered possible comments or strings with mixed
> > +     delimiters.  Bail out and scan forward from a safe position. */

> "lose" and "lossage" are too similar.  Can we have a better name for
> the latter?

OK.  I took the names from, I think, back_comment.

> > +  {
> > +    struct lisp_parse_state state;
> > +    bool adjusted = true;

> Why did you need the braces here?  C99 allows to mix declarations and
> statements, so we no longer need such braces.

OK.

> > +        find_start_value
> > +          = CONSP (state.levelstarts) ? XINT (XCAR (state.levelstarts))
> > +          : state.thislevelstart >= 0 ? state.thislevelstart
> > +          : find_start_value;

> Please use parentheses here for better readability (to clearly show
> which parts belong to which condition).

Yes, it didn't indent well by itself.  Maybe I should raise this with
the CC Mode maintainer.  But yes, I'll put parens in.

> > -Comments are ignored if `parse-sexp-ignore-comments' is non-nil.
> > +Comments are skipped over if `parse-sexp-ignore-comments' is non-nil.

> We nowadays prefer to quote 'like this' in comments and plain text.

OK.

> > -Comments are ignored if `parse-sexp-ignore-comments' is non-nil.
> > +Comments are skipped over if `parse-sexp-ignore-comments' is non-nil.

> Likewise.

> Thanks again for working on this.

I'll make the stylistic corrections, then get working on it again in
earnest.

-- 
Alan Mackenzie (Nuremberg, Germany).