bug#64133: CC Mode 5.35.2 (C/*l); incorrect indentation for an arrays of structs.

bug#64133: CC Mode 5.35.2 (C/*l); incorrect indentation for an arrays of structs.

[Jeff Norden]

I've verified the following behavior with c-version 5.35.2 (emacs-29,
and emacs-30 from git) and with 5.35.1 (emacs 28.2).

Place the following text into a file with a ".c" extension, and load
it with "emacs -Q".
==========================================
#include <stdio.h>
struct three_ints {
  int a, b, c;
};
int main () {
  struct three_ints numbers[]= {
    {0,1,2},
{3,4,5},
    {6,7,8}
  };
  for (int i=0; i<numbers[2].a; i++) {
    printf("ack ");
  }
  printf("\n");
}
==========================================
Move the cursor to the beginning of the line with {3,4,5} and press
<tab> to indent the line.  It will be incorrectly indented 2 spaces
more than the previous line.  Move to the next line, press <tab>,
and you'll see:

  struct three_ints numbers[]= {
    {0,1,2},
      {3,4,5},
        {6,7,8}
  };

Just using <tab> to re-indent the lines will not change anything.
But, if you add or delete white-space before {0,1,2} or {3,4,5} and
press <tab>, it *may* fix things.  It seems to depend on just what you
delete and where the cursor is.  The pattern isn't clear to me.
Once fixed, you get:

  struct three_ints numbers[]= {
    {0,1,2},
    {3,4,5},
    {6,7,8}
  };

At this point it is stable, and <tab> will work correctly.  If you
restart c-mode with "M-x c-mode", the problem re-occurs.

This bug only seems to occur when a loop or similar construct occurs
after the array declaration/initialization.  It doesn't need to
immediately follow, though.

When incorrectly indenting, "C-c C-s" shows 'defun-block-intro' syntax
for the {0,1,2} line, and 'substatement-open' for {3,4,5} and {4,5,6}.
Once it gets fixed, this changes to 'brace-list-intro' and
'brace-entry-open'.

==========================================
For a more "real world" example, start with the example program at the
end of the getopt_long(3) man page:

  https://linux.die.net/man/3/getopt_long

and move the declaration of long_options[] from inside the while loop
to before it.  (This makes long_options a global variable, instead of
local to the loop).

==========================================
I'm afraid I can't offer a patch or suggestion for a fix.  My own
attempts at "parsing" the lisp in cc-engine.el come to an abrupt halt
when my brain receives an urgent "Warning! Meltdown imminent!"
message from my subconscious -- just kidding :-).

However, I did stumble upon the following in the comments for the
`c-inside-bracelist-p' function:

 ;; CONTAINING-SEXP is the buffer pos of the innermost containing
 ;; paren.  NO IT ISN'T!!!  [This function is badly designed, and
 ;; probably needs reformulating without its first argument, ...

Thanks,
-Jeff
----------------------------
  If believing that "function is more important than form" and that "people
  are more important than technology" makes me a Luddite, then so be it!

bug#64133: CC Mode 5.35.2 (C/*l); incorrect indentation for an arrays of structs.

[Eli Zaretskii]

> From: Jeff Norden <norden.jeff@gmail.com>
> Date: Sat, 17 Jun 2023 10:03:24 -0500
> 
> I've verified the following behavior with c-version 5.35.2 (emacs-29,
> and emacs-30 from git) and with 5.35.1 (emacs 28.2).
> 
> Place the following text into a file with a ".c" extension, and load
> it with "emacs -Q".
> ==========================================
> #include <stdio.h>
> struct three_ints {
>   int a, b, c;
> };
> int main () {
>   struct three_ints numbers[]= {
>     {0,1,2},
> {3,4,5},
>     {6,7,8}
>   };
>   for (int i=0; i<numbers[2].a; i++) {
>     printf("ack ");
>   }
>   printf("\n");
> }
> ==========================================
> Move the cursor to the beginning of the line with {3,4,5} and press
> <tab> to indent the line.  It will be incorrectly indented 2 spaces
> more than the previous line.  Move to the next line, press <tab>,
> and you'll see:
> 
>   struct three_ints numbers[]= {
>     {0,1,2},
>       {3,4,5},
>         {6,7,8}
>   };

This source code uses the 'linux' indentation style, so you should do

  M-x c-set-style RET linux RET

after which the problem goes away, AFAICT.

bug#64133: CC Mode 5.35.2 (C/*l); incorrect indentation for an arrays of structs.

[Eli Zaretskii]

> From: Jeff Norden <norden.jeff@gmail.com>
> Date: Sat, 17 Jun 2023 10:43:40 -0500
> 
> > This source code uses the 'linux' indentation style, so you should do
> >
> >   M-x c-set-style RET linux RET
> >
> > after which the problem goes away, AFAICT.
> Eli: Thanks for the quick response.  Changing the style works for me,
> so this can be closed.
> I didn't realize that the style effects parsing, not just the way
> space, etc gets inserted by default.

Thanks, I will let Alan chime in and decide whether this should be
closed.

bug#64133: CC Mode 5.35.2 (C/*l); incorrect indentation for an arrays of structs.

[Jeff Norden]

On Sat, Jun 17, 2023 at 11:27 AM Eli Zaretskii <eliz@gnu.org> wrote:
> Thanks, I will let Alan chime in and decide whether this should be
> closed.
Actually, I now think that changing the style works because the gnu
style has
  (substatement-open . +)
in c-offsets-alist, while linux uses (substatement-open . 0).  With
either style, I initially get substatemnt syntax from "C-c C-s", which
changes to brace-list when I delete space and re-indent.

This issue *doesn't* occur if the opening brace is on its own line,
and it looks like most styles other than gnu do not indent
sub-statements.  So, I still think this bug can be closed, but maybe
the resolution could be "mostly harmless" :-).

bug#64133: CC Mode 5.35.2 (C/*l); incorrect indentation for an arrays of structs.

[Alan Mackenzie]

Hello, Jeff

Thanks for taking the trouble to report this bug.

On Sat, Jun 17, 2023 at 12:36:30 -0500, Jeff Norden wrote:
> On Sat, Jun 17, 2023 at 11:27 AM Eli Zaretskii <eliz@gnu.org> wrote:
> > Thanks, I will let Alan chime in and decide whether this should be
> > closed.

I think, not yet.

> Actually, I now think that changing the style works because the gnu
> style has
>   (substatement-open . +)
> in c-offsets-alist, while linux uses (substatement-open . 0).  With
> either style, I initially get substatemnt syntax from "C-c C-s", which
> changes to brace-list when I delete space and re-indent.

> This issue *doesn't* occur if the opening brace is on its own line,
> and it looks like most styles other than gnu do not indent
> sub-statements.  So, I still think this bug can be closed, but maybe
> the resolution could be "mostly harmless" :-).

I've edebugged through c-guess-basic-syntax and subroutines, and it's
now clear there's a bug in the handling of a cache.  The cache, called
c-laomib-cache (with "laomib" standing for "looking at or maybe in
bracelist") speeds up the handling of very large brace lists which
frequently occur in generated .h files.  Without the cache, these would
have to be scanned over repeatedly, which made C Mode very slow for
these files.

I think I should be able to fix this in the next day or three.

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#64133: CC Mode 5.35.2 (C/*l); incorrect indentation for an arrays of structs.

[Jeff Norden]

On Sun, Jun 18, 2023 at 8:13 AM Alan Mackenzie wrote:
>...
> > > Thanks, I will let Alan chime in and decide whether this should be
> > > closed.
>
> I think, not yet.
>...
> I think I should be able to fix this in the next day or three.

Hi Alan

I don't know if this might help, but here is one more thing I found,
which I can't explain at all.

Tab is normally bound to c-indent-line-or-region.  You can't directly
re-bind tab to c-indent-line.  However, if you create an interactive
version with:

  (defun cil() (interactive) (c-indent-line))

and bind it to tab, it has the exact same behavior.  This eliminates
several levels of function calls between c-indent-line-or-region and
c-indent-line.

So, if you open the example with emacs -Q, define cil and bind it to
tab with "M-x local-set-key", you can "fix" the syntax by moving to
the beginning of the {3,4,5} line and pressing:
  <tab><backspace><backspace><tab>
At this point "C-c C-s" shows brace-list syntax.

BUT, if you start again, and instead of defining "cil', just replace
each <tab> with "M-: (c-indent-line)<ret>", it does *not* change the
syntax that "C-c C-s" shows.  For some reason that I don't understand,
it seems that the minibuffer read affects the behavior.  To see this,
change the defun for "cil" by adding:

  (read-from-minibuffer "press <ret>")

after (interactive).  You now need to hit <ret> after each <tab>, but
I *cannot* get the syntax for the line to change with this function.
-Jeff

bug#64133: CC Mode 5.35.2 (C/*l); incorrect indentation for an arrays of structs.

[Jeff Norden]

The only reason the minibuffer-read affects things is timing.  This
seems obvious now, but somehow it didn't occur to me until after I
sent the previous message.  If I do

  <tab><backspace><backspace> ..wait a few seconds.. <tab>

then the syntax does *not* change to brace-list.  I tried changing
c-progress-interval, but that didn't seem to affect things.  Maybe
there is another timer, or maybe a race condition involved somehow.

Hope this helps,
-Jeff

bug#64133: CC Mode 5.35.2 (C/*l); incorrect indentation for an arrays of structs.

[Alan Mackenzie]

Hello, Jeff.

On Sun, Jun 18, 2023 at 12:23:58 -0500, Jeff Norden wrote:
> The only reason the minibuffer-read affects things is timing.  This
> seems obvious now, but somehow it didn't occur to me until after I
> sent the previous message.  If I do

>   <tab><backspace><backspace> ..wait a few seconds.. <tab>

> then the syntax does *not* change to brace-list.  I tried changing
> c-progress-interval, but that didn't seem to affect things.  Maybe
> there is another timer, or maybe a race condition involved somehow.

No, I think it really is the cache bug I mentioned in my last post.
Caches are wonderful things when they work properly, but the utter devil
to debug when they don't.

I've currently got a CC Mode version with this cache corrected, such
that it parses your test file correctly, but I want to make sure the
correction is correct before I send you a patch and release the fix.

I'll be back again soon.

> Hope this helps,
> -Jeff

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#64133: CC Mode 5.35.2 (C/*l); incorrect indentation for an arrays of structs.

[Jeff Norden]

On Mon, Jun 19, 2023 at 6:46 AM Alan Mackenzie <acm@muc.de> wrote:
> No, I think it really is the cache bug I mentioned in my last post.

I agree.  In fact, a "brute force" fix is to use advice-add to reset
c-laomib-cache to nil before each call to c-indent-line.  What puzzled
me is that certain buffer changes also fix the issue, and that timing
makes a difference in when and how that happens.  I'm now guessing
that the bug is triggered by calls from the fontification system.  If
I start "emacs -Q", and do `M-x global-font-lock-mode' to turn all
fontifcation off, then "C-c C-s" shows correct syntax when I open the
example file.

Thanks again,
-Jeff

bug#64133: CC Mode 5.35.2 (C/*l); incorrect indentation for an arrays of structs.

[Alan Mackenzie]

Hello again, Jeff.

On Mon, Jun 19, 2023 at 11:46:32 +0000, Alan Mackenzie wrote:
> On Sun, Jun 18, 2023 at 12:23:58 -0500, Jeff Norden wrote:
> > The only reason the minibuffer-read affects things is timing.  This
> > seems obvious now, but somehow it didn't occur to me until after I
> > sent the previous message.  If I do

> >   <tab><backspace><backspace> ..wait a few seconds.. <tab>

> > then the syntax does *not* change to brace-list.  I tried changing
> > c-progress-interval, but that didn't seem to affect things.  Maybe
> > there is another timer, or maybe a race condition involved somehow.

> No, I think it really is the cache bug I mentioned in my last post.
> Caches are wonderful things when they work properly, but the utter devil
> to debug when they don't.

> I've currently got a CC Mode version with this cache corrected, such
> that it parses your test file correctly, but I want to make sure the
> correction is correct before I send you a patch and release the fix.

> I'll be back again soon.

OK, here's the patch which I think fixes the bug completely.  Would you
please apply this patch to your ..../lisp/progmodes/cc-engine.el,
byte-compile (or native-compile) that file, and then try it out on your
real code.  (In the unlikely event that you want help with the patching
or compilation, feel free to send me personal email.)  Then, please
confirm that the bug is fixed, or tell me what's still not working.

Thanks!



diff -r 2cd57c62754a cc-engine.el
--- a/cc-engine.el	Sat Jun 17 12:45:10 2023 +0000
+++ b/cc-engine.el	Wed Jun 21 10:27:21 2023 +0000
@@ -12816,11 +12816,19 @@
 (defvar c-laomib-cache nil)
 (make-variable-buffer-local 'c-laomib-cache)
 
-(defun c-laomib-get-cache (containing-sexp)
-  ;; Get an element from `c-laomib-cache' matching CONTAINING-SEXP.
+(defun c-laomib-get-cache (containing-sexp start)
+  ;; Get an element from `c-laomib-cache' matching CONTAINING-SEXP, and which
+  ;; is suitable for start postiion START.
   ;; Return that element or nil if one wasn't found.
-  (let ((elt (assq containing-sexp c-laomib-cache)))
-    (when elt
+  (let ((ptr c-laomib-cache)
+	elt)
+    (while
+	(and ptr
+	     (setq elt (car ptr))
+	     (or (not (eq (car elt) containing-sexp))
+		 (< start (car (cddr elt)))))
+      (setq ptr (cdr ptr)))
+    (when ptr
       ;; Move the fetched `elt' to the front of the cache.
       (setq c-laomib-cache (delq elt c-laomib-cache))
       (push elt c-laomib-cache)
@@ -12832,18 +12840,26 @@
   ;; the components of the new element (see comment for `c-laomib-cache').
   ;; The return value is of no significance.
   (when lim
-    (let ((old-elt (assq lim c-laomib-cache))
-	  ;; (elt (cons containing-sexp (cons start nil)))
+    (let (old-elt
 	  (new-elt (list lim start end result))
 	  big-ptr
 	  (cur-ptr c-laomib-cache)
-	  togo (size 0) cur-size
-	  )
-      (if old-elt (setq c-laomib-cache (delq old-elt c-laomib-cache)))
+	  togo (size 0) cur-size)
+
+      ;; If there is an elt which overlaps with the new element, remove it.
+      (while
+	  (and cur-ptr
+	       (setq old-elt (car cur-ptr))
+	       (or (not (eq (car old-elt) lim))
+		   (not (and (> start (car (cddr old-elt)))
+			     (<= start (cadr old-elt))))))
+	(setq cur-ptr (cdr cur-ptr)))
+      (when (and cur-ptr old-elt)
+	(setq c-laomib-cache (delq old-elt c-laomib-cache)))
 
       (while (>= (length c-laomib-cache) 4)
 	;; We delete the least recently used elt which doesn't enclose START,
-	;; or..
+	;; or ...
 	(dolist (elt c-laomib-cache)
 	  (if (or (<= start (cadr elt))
 		  (> start (car (cddr elt))))
@@ -12851,8 +12867,10 @@
 
 	;; ... delete the least recently used elt which isn't the biggest.
 	(when (not togo)
+	  (setq cur-ptr c-laomib-cache)
 	  (while (cdr cur-ptr)
-	    (setq cur-size (- (nth 2 (cadr cur-ptr)) (car (cadr cur-ptr))))
+	    (setq cur-size (- (cadr (cadr cur-ptr))
+			      (car (cddr (cadr cur-ptr)))))
 	    (when (> cur-size size)
 	      (setq size cur-size
 		    big-ptr cur-ptr))
@@ -13044,7 +13062,7 @@
 	(goto-char pos)
 	(when (eq braceassignp 'dontknow)
 	  (let* ((cache-entry (and containing-sexp
-				   (c-laomib-get-cache containing-sexp)))
+				   (c-laomib-get-cache containing-sexp pos)))
 		 (lim2 (or (cadr cache-entry) lim))
 		 sub-bassign-p)
 	    (if cache-entry
@@ -13066,6 +13084,8 @@
 					    )
 			(setq braceassignp (nth 3 cache-entry))
 			(goto-char (nth 2 cache-entry)))
+		    (c-laomib-put-cache containing-sexp
+					start (point) sub-bassign-p)
 		    (setq braceassignp sub-bassign-p)))
 		 (t))
 


> > Hope this helps,
> > -Jeff

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#64133: CC Mode 5.35.2 (C/*l); incorrect indentation for an arrays of structs.

[Jeff Norden]

On Wed, Jun 21, 2023 at 5:39 AM Alan Mackenzie <acm@muc.de> wrote:
> Hello again, Jeff.
>
> OK, here's the patch which I think fixes the bug completely.  Would you
> please apply this patch  ...   Then, please confirm that the bug is fixed, or
> tell me what's still not working.
Hi Alan,

Thanks for taking care of this so quickly.  The patch seems to work
perfectly for me.  I've tried it with 29.0.91 (2nd emacs-29 pretest)
and with my current git build of emacs from the master branch (which
is from June 11).  Patch gives differing "offset" messages, but all of
the "hunks" succeed in both cases.  I haven't built the 3rd emacs-29
pretest yet, but I can't foresee that there will be a problem.  I will
be using this some more over the next few days.  I'll post again if I
notice any issues, but I don't expect to.

-Jeff

bug#64133: CC Mode 5.35.2 (C/*l); incorrect indentation for an arrays of structs.

[Alan Mackenzie]

Hello, Jeff.

On Wed, Jun 21, 2023 at 13:43:20 -0500, Jeff Norden wrote:
> On Wed, Jun 21, 2023 at 5:39 AM Alan Mackenzie <acm@muc.de> wrote:
> > OK, here's the patch which I think fixes the bug completely.  Would you
> > please apply this patch  ...   Then, please confirm that the bug is fixed, or
> > tell me what's still not working.
> Hi Alan,

> Thanks for taking care of this so quickly.  The patch seems to work
> perfectly for me.  I've tried it with 29.0.91 (2nd emacs-29 pretest)
> and with my current git build of emacs from the master branch (which
> is from June 11).  Patch gives differing "offset" messages, but all of
> the "hunks" succeed in both cases.  I haven't built the 3rd emacs-29
> pretest yet, but I can't foresee that there will be a problem.  I will
> be using this some more over the next few days.  I'll post again if I
> notice any issues, but I don't expect to.

Thanks for such comprehensive testing!  I've now committed the patch to
the Emacs master branch (it is too intricate a patch to go to the
release branch at this late stage), and I'm closing the bug with this
post.

If any other problems become apparent in this area (or anywhere else),
feel free to raise another bug report.

> -Jeff

-- 
Alan Mackenzie (Nuremberg, Germany).