bug#37910: CC Mode 5.33.2 (C++//l); CC-mode inconsistently indents everything as topmost-intro after a while

[Eric Scrivner <eric.t.scrivner@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 8469 bytes --]

Hi Andrew,

Thanks so much for the patch and explanation. I applied it locally and
while the patch didn't seem to have much effect, setting
`open-paren-in-column-0-is-defun-start` to `nil` seems to have done the
trick here without any noticeable impact on performance. It seems as you've
said that there may be some issues in the heuristic used for performance
optimization here.

I will let you know if any other issues surface, but it appears this has
fixed my issue.

On Wed, Nov 13, 2019 at 10:39 AM Alan Mackenzie <acm@muc.de> wrote:

> Hello again, Eric.
>
> On Sun, Nov 10, 2019 at 09:47:35 -0800, Eric Scrivner wrote:
> > I've applied the patch and run emacs with it. Unfortunately, it doesn't
> > seem to solve the issue. One the bright side, I believe I had buffers
> which
> > can reliably reproduce the issue (at least locally). I've copied one such
> > file below. The issue can be reproduced by simply going to the end of
> > `main` and attempting to indent the `SQL_Quit()` statement. At first it
> > attempts to overindent the line as `statement-cont`, then if I go up and
> > attempt to indent the `PlatformLog` in the else statement then return to
> > the SDL_Quit and indent it, it has become `topmost-intro`. Apologies for
> > the long length of the file, but hopefully it can help with reproduction:
>
> I've had a look at the bug and there was a non-sensical function in CC
> Mode fouling things up.  In the patch below, I've rewritten this
> function.  Could I ask you please to apply the new patch (to the
> original Emacs 26.3 version off cc-engine.el), byte-compile it and test
> it.  Then please let me know how it goes.  However ....
>
> I would strongly advise you to set the Emacs variable
> open-parens-in-column-0-is-defun-start to nil.  Whether for C++ Mode in
> general (in a hook function), for this one file in particular (in a
> Local Variables: section at the end of the file), or globally.
>
> With open-p......-start left at its default of t, Emacs will search
> backward for the beginning of a function by searching for an open
> paren/brace/bracket at column 0.  It deems the found paren/brace/bracket
> the start of a function even if that p/b/b is inside a string or
> function.  This seems likely to happen in your test file, where you've
> got such braces at column 0 inside two raw strings.
>
> This variable codifies a difficult dilemma for Emacs.  Without the paren
> in column 0 heuristic, the beginning of function search would have to
> scan to the beginning of the buffer every time which is somewhat slow
> (perhaps less slow now on modern PCs, but still slow).  With the
> heuristic, analysis problems happen when there are braces at col 0 which
> aren't BO functions (e.g., in C++ Mode when things inside a namespace
> start at column 0, a popular convention).  For a time, o-p-i-c-0-i-d-s
> was spiked to nil by CC Mode, but this led to complaints of slowness.
>
> Anyhow, here's the improved patch.  I look forward to hearing back from
> you.  Thanks for all the time you've spent on this bug.
>
>
>
> diff -r 2783baa48d44 cc-engine.el
> --- a/cc-engine.el      Fri Oct 25 20:00:14 2019 +0000
> +++ b/cc-engine.el      Wed Nov 13 18:13:11 2019 +0000
> @@ -3371,19 +3371,35 @@
>    (or (car (c-state-literal-at pos))
>        pos))
>
> -(defsubst c-state-cache-non-literal-place (pos state)
> -  ;; Return a position outside of a string/comment/macro at or before POS.
> -  ;; STATE is the parse-partial-sexp state at POS.
> -  (let ((res (if (or (nth 3 state)     ; in a string?
> -                    (and (nth 4 state)
> -                         (not (eq (nth 7 state) 'syntax-table)))) ; in a
> comment?
> -                (nth 8 state)
> -              pos)))
> +(defun c-state-cache-lower-good-pos (here pos state)
> +  ;; Return a good pos (in the sense of `c-state-cache-good-pos') at the
> +  ;; lowest[*] position between POS and HERE which is syntactically
> equivalent
> +  ;; to HERE.  This position may be HERE itself.  POS is before HERE in
> the
> +  ;; buffer.
> +  ;; [*] We don't actually always determine this exact position, since
> this
> +  ;; would require a disproportionate amount of work, given that this
> function
> +  ;; deals only with a corner condition, and POS and HERE are typically on
> +  ;; adjacent lines.  We actually return either POS, when POS is a good
> +  ;; position, HERE otherwise.  Exceptionally, when POS is in a comment,
> but
> +  ;; HERE not, we can return the position of the end of the comment.
> +  (let (s)
>      (save-excursion
> -      (goto-char res)
> -      (if (c-beginning-of-macro)
> -         (point)
> -       res))))
> +      (goto-char pos)
> +      (when (nth 8 state)      ; POS in a comment or string.  Move out of
> it.
> +       (setq s (parse-partial-sexp pos here nil nil state 'syntax-table))
> +       (when (< (point) here)
> +         (setq pos (point)
> +               state s)))
> +      (if (eq (point) here)            ; HERE is in the same literal as
> POS
> +         pos
> +       (setq s (parse-partial-sexp pos here (1+ (car state)) nil state
> nil))
> +       (cond
> +        ((> (car s) (car state))  ; Moved into a paren between POS and
> HERE
> +         here)
> +        ((not (eq (nth 6 s) (car state))) ; Moved out of a paren between
> POS
> +                                       ; and HERE
> +         here)
> +        (t pos))))))
>
>  ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
>  ;; Stuff to do with point-min, and coping with any literal there.
> @@ -3690,7 +3706,13 @@
>                                                         ; brace pair.
>             (setq c-state-cache nil
>                   c-state-cache-good-pos c-state-min-scan-pos)
> -         (setcdr ptr nil)
> +         ;; Do not alter the original `c-state-cache' structure, since
> there
> +         ;; may be a loop suspended which is looping through that
> structure.
> +         ;; This may have been the cause of bug #37910.
> +         (let ((cdr-ptr (cdr ptr)))
> +           (setcdr ptr nil)
> +           (setq c-state-cache (copy-sequence c-state-cache))
> +           (setcdr ptr cdr-ptr))
>           (setq c-state-cache-good-pos (1+ (c-state-cache-top-lparen))))
>         )))
>
> @@ -3793,11 +3815,12 @@
>                 (setq new-cons (cons bra (1+ ce)))
>                 (cond
>                  ((consp (car c-state-cache))
> -                 (setcar c-state-cache new-cons))
> +                 (setq c-state-cache (cons new-cons (cdr c-state-cache))))
>                  ((and (numberp (car c-state-cache)) ; probably never
> happens
>                        (< ce (car c-state-cache)))
> -                 (setcdr c-state-cache
> -                         (cons new-cons (cdr c-state-cache))))
> +                 (setq c-state-cache
> +                       (cons (car c-state-cache)
> +                             (cons new-cons (cdr c-state-cache)))))
>                  (t (setq c-state-cache (cons new-cons c-state-cache)))))
>
>             ;; We haven't found a brace pair.  Record this in the cache.
> @@ -3998,7 +4021,7 @@
>         (when (and c-state-cache
>                    (consp (car c-state-cache))
>                    (> (cdar c-state-cache) upper-lim))
> -         (setcar c-state-cache (caar c-state-cache))
> +         (setq c-state-cache (cons (caar c-state-cache) (cdr
> c-state-cache)))
>           (setq scan-back-pos (car c-state-cache)
>                 cons-separated t))
>
> @@ -4135,7 +4158,7 @@
>        ;; knowledge of what's inside these braces, we have no alternative
> but
>        ;; to direct the caller to scan the buffer from the opening brace.
>        (setq pos (caar c-state-cache))
> -      (setcar c-state-cache pos)
> +      (setq c-state-cache (cons pos (cdr c-state-cache)))
>        (list (1+ pos) pos t)) ; return value.  We've just converted a
> brace pair
>                              ; entry into a { entry, so the caller needs to
>                              ; search for a brace pair before the {.
> @@ -4385,7 +4408,7 @@
>        (setq c-state-cache-good-pos
>             (if (and bopl-state
>                      (< good-pos (- here c-state-cache-too-far)))
> -               (c-state-cache-non-literal-place here-bopl bopl-state)
> +               (c-state-cache-lower-good-pos here here-bopl bopl-state)
>               good-pos)))
>
>       ((eq strategy 'backward)
>
>
> --
> Alan Mackenzie (Nuremberg, Germany).
>

[-- Attachment #2: Type: text/html, Size: 9957 bytes --]