emacs-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Eli Zaretskii
Subject: Re: scratch/fontify-open-string. [Was: CC Mode and electric-pair "problem".]
Date: Sun, 15 Jul 2018 18:13:15 +0300

> From: Stephen Leake <address@hidden>
> 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):

  "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.

> +#define DEC_AT                                                  \

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

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

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

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

A single line doesn't need braces.

> +      /* 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.

> + 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?

> +  {
> +    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.

> +        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).

> -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.

> -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.



reply via email to

[Prev in Thread] Current Thread [Next in Thread]