[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: |
Alan Mackenzie |
Subject: |
Re: scratch/fontify-open-string. [Was: CC Mode and electric-pair "problem".] |
Date: |
Sun, 15 Jul 2018 18:45:38 +0000 |
User-agent: |
Mutt/1.9.4 (2018-02-28) |
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 <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):
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).
- Re: CC Mode and electric-pair "problem"., (continued)
Re: CC Mode and electric-pair "problem"., Alan Mackenzie, 2018/07/01
Re: CC Mode and electric-pair "problem"., João Távora, 2018/07/01
Re: CC Mode and electric-pair "problem"., Eli Zaretskii, 2018/07/01
- scratch/fontify-open-string. [Was: CC Mode and electric-pair "problem".], Alan Mackenzie, 2018/07/01
- Re: scratch/fontify-open-string. [Was: CC Mode and electric-pair "problem".], Stephen Leake, 2018/07/08
- Re: scratch/fontify-open-string. [Was: CC Mode and electric-pair "problem".], Stephen Leake, 2018/07/15
- Re: scratch/fontify-open-string. [Was: CC Mode and electric-pair "problem".], Eli Zaretskii, 2018/07/15
- Re: scratch/fontify-open-string. [Was: CC Mode and electric-pair "problem".],
Alan Mackenzie <=
- Indentation of ?: in C-mode (was: scratch/fontify-open-string. [Was: CC Mode and electric-pair "problem".]), Stefan Monnier, 2018/07/15
- Re: Indentation of ?: in C-mode (was: scratch/fontify-open-string. [Was: CC Mode and electric-pair "problem".]), Eli Zaretskii, 2018/07/16
- Re: Indentation of ?: in C-mode, Stefan Monnier, 2018/07/16
Re: scratch/fontify-open-string. [Was: CC Mode and electric-pair "problem".], Alan Mackenzie, 2018/07/15
Re: scratch/fontify-open-string. [Was: CC Mode and electric-pair "problem".], Stephen Leake, 2018/07/16
Re: CC Mode and electric-pair "problem"., Eli Zaretskii, 2018/07/01
Re: CC Mode and electric-pair "problem"., Paul Eggert, 2018/07/01
Re: CC Mode and electric-pair "problem"., Stephen Leake, 2018/07/06