GNU bug report logs - #61558
29.0.60; Indentation with c-ts-mode doesn't work in code guarded by #ifdef..#endif

Previous Next

To reply to this bug, email your comments to 61558 AT debbugs.gnu.org.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: bug-gnu-emacs <at> gnu.org
Subject: 29.0.60; Indentation with c-ts-mode doesn't work in code guarded by
 #ifdef..#endif
Date: Thu, 16 Feb 2023 22:48:09 +0200

To reproduce:

  emacs -Q
  C-x C-f src/dispnew.c RET
  C-u 265 M-g g
  C-e
  RET

The code around there looks like this:

static struct glyph_matrix *
new_glyph_matrix (struct glyph_pool *pool)
{
  struct glyph_matrix *result = xzalloc (sizeof *result);

#if defined GLYPH_DEBUG && defined ENABLE_CHECKING
  /* Increment number of allocated matrices.  This count is used
     to detect memory leaks.  */
  ++glyph_matrix_count;  <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
#endif

Line 265 is the one indicated with "<<<<<<".  Pressing RET goes to
column 0, not column 2 as expected.  It looks like indentation doesn't
work in code fragments that are guarded by #ifdef..#endif preprocessor
conditions.  I tried several such places, for example, lines 295, 796,
1338, 1360, 1745, 2401, 2615, and many more.

Strangely, in other places indentation does work: lines 1069, 3119.


In GNU Emacs 29.0.60 (build 326, i686-pc-mingw32) of 2023-02-16 built on
 HOME-C4E4A596F7
Repository revision: f1f571e72ae10285762d3a941e56f7c4048272af
Repository branch: emacs-29
Windowing system distributor 'Microsoft Corp.', version 5.1.2600
System Description: Microsoft Windows XP Service Pack 3 (v5.1.0.2600)

Configured using:
 'configure -C --prefix=/d/usr --with-wide-int
 --enable-checking=yes,glyphs 'CFLAGS=-O0 -gdwarf-4 -g3''

Configured features:
ACL GIF GMP GNUTLS HARFBUZZ JPEG JSON LCMS2 LIBXML2 MODULES NOTIFY
W32NOTIFY PDUMPER PNG RSVG SOUND SQLITE3 THREADS TIFF
TOOLKIT_SCROLL_BARS TREE_SITTER WEBP XPM ZLIB

Important settings:
  value of $LANG: ENU
  locale-coding-system: cp1255

Major mode: C

Minor modes in effect:
  tooltip-mode: t
  global-eldoc-mode: t
  show-paren-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  line-number-mode: t
  indent-tabs-mode: t
  transient-mark-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug message mailcap yank-media puny dired
dired-loaddefs rfc822 mml mml-sec password-cache epa derived epg rfc6068
epg-config gnus-util text-property-search time-date subr-x mm-decode
mm-bodies mm-encode mail-parse rfc2231 mailabbrev gmm-utils mailheader
sendmail rfc2047 rfc2045 ietf-drums mm-util mail-prsvr mail-utils
c-ts-mode c-ts-common treesit cl-seq vc-git diff-mode easy-mmode vc
vc-dispatcher bug-reference byte-opt gv bytecomp byte-compile cc-mode
cc-fonts cc-guess cc-menus cc-cmds cc-styles cc-align cc-engine cc-vars
cc-defs cl-loaddefs cl-lib rmc iso-transl tooltip cconv eldoc paren
electric uniquify ediff-hook vc-hooks lisp-float-type elisp-mode mwheel
dos-w32 ls-lisp disp-table term/w32-win w32-win w32-vars term/common-win
tool-bar dnd fontset image regexp-opt fringe tabulated-list replace
newcomment text-mode lisp-mode prog-mode register page tab-bar menu-bar
rfn-eshadow isearch easymenu timer select scroll-bar mouse jit-lock
font-lock syntax font-core term/tty-colors frame minibuffer nadvice seq
simple cl-generic indonesian philippine cham georgian utf-8-lang
misc-lang vietnamese tibetan thai tai-viet lao korean japanese eucjp-ms
cp51932 hebrew greek romanian slovak czech european ethiopic indian
cyrillic chinese composite emoji-zwj charscript charprop case-table
epa-hook jka-cmpr-hook help abbrev obarray oclosure cl-preloaded button
loaddefs theme-loaddefs faces cus-face macroexp files window
text-properties overlay sha1 md5 base64 format env code-pages mule
custom widget keymap hashtable-print-readable backquote threads
w32notify w32 lcms2 multi-tty make-network-process emacs)

Memory information:
((conses 16 80960 11004)
 (symbols 48 9604 0)
 (strings 16 28428 2568)
 (string-bytes 1 916238)
 (vectors 16 15765)
 (vector-slots 8 208391 13142)
 (floats 8 28 52)
 (intervals 40 1627 87)
 (buffers 888 11))

Message #8 received at 61558 <at> debbugs.gnu.org (full text, mbox):

From: Theodor Thornhill <theo <at> thornhill.no>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 61558 <at> debbugs.gnu.org
Subject: Re: bug#61558: 29.0.60; Indentation with c-ts-mode doesn't work in
 code guarded by #ifdef..#endif
Date: Fri, 17 Feb 2023 20:23:33 +0100

Eli Zaretskii <eliz <at> gnu.org> writes:

> To reproduce:
>
>   emacs -Q
>   C-x C-f src/dispnew.c RET
>   C-u 265 M-g g
>   C-e
>   RET
>
> The code around there looks like this:
>
> static struct glyph_matrix *
> new_glyph_matrix (struct glyph_pool *pool)
> {
>   struct glyph_matrix *result = xzalloc (sizeof *result);
>
> #if defined GLYPH_DEBUG && defined ENABLE_CHECKING
>   /* Increment number of allocated matrices.  This count is used
>      to detect memory leaks.  */
>   ++glyph_matrix_count;  <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
> #endif
>
> Line 265 is the one indicated with "<<<<<<".  Pressing RET goes to
> column 0, not column 2 as expected.  It looks like indentation doesn't
> work in code fragments that are guarded by #ifdef..#endif preprocessor
> conditions.  I tried several such places, for example, lines 295, 796,
> 1338, 1360, 1745, 2401, 2615, and many more.
>

Yep. We need rules for these in particular.  They aren't really
straightforward because the expected indent style varies, afaict.  For
example:

#ifdef WINDOWSNT
#include "w32.h"
#endif

compared to

#if defined GLYPH_DEBUG && defined ENABLE_CHECKING
  /* Increment number of allocated matrices.  This count is used
     to detect memory leaks.  */
  ++glyph_matrix_count;
#endif


Is it a correct assuption to think that whatever is inside one of these
if-blocks should indent according to their grand-parents rule?

In this case:


static struct glyph_matrix *
new_glyph_matrix (struct glyph_pool *pool)
{
  struct glyph_matrix *result = xzalloc (sizeof *result);

#if defined GLYPH_DEBUG && defined ENABLE_CHECKING
  /* Increment number of allocated matrices.  This count is used
     to detect memory leaks.  */
  ++glyph_matrix_count;
#endif

  /* Set pool and return.  */
  result->pool = pool;
  return result;
}

  ++glyph_matrix_count;

is indented one step from the compound_statement node, right?  So we
need a way to "ignore" the parents indentation.


> Strangely, in other places indentation does work: lines 1069, 3119.
>

Yeah, in these cases we have something other than the preproc directive
itself to indent from.

Theo

Message #11 received at 61558 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Theodor Thornhill <theo <at> thornhill.no>
Cc: 61558 <at> debbugs.gnu.org
Subject: Re: bug#61558: 29.0.60; Indentation with c-ts-mode doesn't work in
 code guarded by #ifdef..#endif
Date: Fri, 17 Feb 2023 21:32:49 +0200

> From: Theodor Thornhill <theo <at> thornhill.no>
> Cc: 61558 <at> debbugs.gnu.org
> Date: Fri, 17 Feb 2023 20:23:33 +0100
> 
> #if defined GLYPH_DEBUG && defined ENABLE_CHECKING
>   /* Increment number of allocated matrices.  This count is used
>      to detect memory leaks.  */
>   ++glyph_matrix_count;
> #endif
> 
> 
> Is it a correct assuption to think that whatever is inside one of these
> if-blocks should indent according to their grand-parents rule?

Yes.  Basically, a cpp macro definition is like a comment: it
disappears when cpp processes it.  So, from the language POV, it
doesn't exist.

> In this case:
> 
> 
> static struct glyph_matrix *
> new_glyph_matrix (struct glyph_pool *pool)
> {
>   struct glyph_matrix *result = xzalloc (sizeof *result);
> 
> #if defined GLYPH_DEBUG && defined ENABLE_CHECKING
>   /* Increment number of allocated matrices.  This count is used
>      to detect memory leaks.  */
>   ++glyph_matrix_count;
> #endif
> 
>   /* Set pool and return.  */
>   result->pool = pool;
>   return result;
> }
> 
>   ++glyph_matrix_count;
> 
> is indented one step from the compound_statement node, right?

Sorry: what is the compound_statement node in this case?

> > Strangely, in other places indentation does work: lines 1069, 3119.
> >
> 
> Yeah, in these cases we have something other than the preproc directive
> itself to indent from.

Preprocessor directives should have no effect whatsoever on code
indentation.

Message #14 received at 61558 <at> debbugs.gnu.org (full text, mbox):

From: Theodor Thornhill <theo <at> thornhill.no>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 61558 <at> debbugs.gnu.org
Subject: Re: bug#61558: 29.0.60; Indentation with c-ts-mode doesn't work in
 code guarded by #ifdef..#endif
Date: Fri, 17 Feb 2023 20:49:23 +0100

[Message part 1 (text/plain, inline)]

Eli Zaretskii <eliz <at> gnu.org> writes:

>> From: Theodor Thornhill <theo <at> thornhill.no>
>> Cc: 61558 <at> debbugs.gnu.org
>> Date: Fri, 17 Feb 2023 20:23:33 +0100
>> 
>> #if defined GLYPH_DEBUG && defined ENABLE_CHECKING
>>   /* Increment number of allocated matrices.  This count is used
>>      to detect memory leaks.  */
>>   ++glyph_matrix_count;
>> #endif
>> 
>> 
>> Is it a correct assuption to think that whatever is inside one of these
>> if-blocks should indent according to their grand-parents rule?
>
> Yes.  Basically, a cpp macro definition is like a comment: it
> disappears when cpp processes it.  So, from the language POV, it
> doesn't exist.
>
>> In this case:
>> 
>> 
>> static struct glyph_matrix *
>> new_glyph_matrix (struct glyph_pool *pool)
>> {
>>   struct glyph_matrix *result = xzalloc (sizeof *result);
>> 
>> #if defined GLYPH_DEBUG && defined ENABLE_CHECKING
>>   /* Increment number of allocated matrices.  This count is used
>>      to detect memory leaks.  */
>>   ++glyph_matrix_count;
>> #endif
>> 
>>   /* Set pool and return.  */
>>   result->pool = pool;
>>   return result;
>> }
>> 
>>   ++glyph_matrix_count;
>> 
>> is indented one step from the compound_statement node, right?
>
> Sorry: what is the compound_statement node in this case?
>

compound_statement is a {} block.

>> > Strangely, in other places indentation does work: lines 1069, 3119.
>> >
>> 
>> Yeah, in these cases we have something other than the preproc directive
>> itself to indent from.
>
> Preprocessor directives should have no effect whatsoever on code
> indentation.

Right, thanks.

Can you test this patch?  It seems to work for me, but I'm no C expert.

Theo

[0001-Cleanup-preproc-indent-for-c-ts-mode.patch (text/x-patch, attachment)]

Message #17 received at 61558 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Theodor Thornhill <theo <at> thornhill.no>
Cc: 61558 <at> debbugs.gnu.org
Subject: Re: bug#61558: 29.0.60; Indentation with c-ts-mode doesn't work in
 code guarded by #ifdef..#endif
Date: Sat, 18 Feb 2023 11:53:49 +0200

> From: Theodor Thornhill <theo <at> thornhill.no>
> Cc: 61558 <at> debbugs.gnu.org
> Date: Fri, 17 Feb 2023 20:49:23 +0100
> 
> Can you test this patch?  It seems to work for me, but I'm no C expert.

Thanks, this is much better.  However, the handling of #if (as opposed
to #ifdef) is still incorrect.  For example, go to the end of line
3376 of dispnew.c and type RET: point goes to column zero, not column
2.  If you convert "#if" to #ifdef", the behavior becomes correct.
Same problem exists with #elif.

Doesn't tree-sitter grammar consider #if and #elif preprocessor nodes?

Also, there's a problem with #define in the middle of code.  For
example, here:

  /* Put a `display' property on the string for the captions to display,
     put a `menu_item' property on tab-bar items with a value that
     is the index of the item in F's tab-bar item vector.  */
  for (i = 0; i < f->n_tab_bar_items; ++i)
    {
#define PROP(IDX) \
  AREF (f->tab_bar_items, i * TAB_BAR_ITEM_NSLOTS + (IDX))

      caption = Fcopy_sequence (PROP (TAB_BAR_ITEM_CAPTION));  <<<<<<<<<<

      /* Put a `display' text property on the string for the caption to
	 display.  Put a `menu-item' property on the string that gives
	 the start of this item's properties in the tab-bar items
	 vector.  */
      AUTO_LIST2 (props, Qmenu_item, make_fixnum (i * TAB_BAR_ITEM_NSLOTS));

      Fadd_text_properties (make_fixnum (0), make_fixnum (SCHARS (caption)),
			    props, caption);

      f->desired_tab_bar_string =
	concat2 (f->desired_tab_bar_string, caption);

#undef PROP <<<<<<<<<<<<<<<<<<<<
    }

Typing RET at the end of the two marked lines goes to column zero
instead of the expected non-zero column.  So it sounds like #define
and #undef are also not handled correctly yet.

Message #20 received at 61558 <at> debbugs.gnu.org (full text, mbox):

From: Theodor Thornhill <theo <at> thornhill.no>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 61558 <at> debbugs.gnu.org
Subject: Re: bug#61558: 29.0.60; Indentation with c-ts-mode doesn't work in
 code guarded by #ifdef..#endif
Date: Sat, 18 Feb 2023 22:11:49 +0100

Eli Zaretskii <eliz <at> gnu.org> writes:

>> From: Theodor Thornhill <theo <at> thornhill.no>
>> Cc: 61558 <at> debbugs.gnu.org
>> Date: Fri, 17 Feb 2023 20:49:23 +0100
>> 
>> Can you test this patch?  It seems to work for me, but I'm no C expert.
>
> Thanks, this is much better.  However, the handling of #if (as opposed
> to #ifdef) is still incorrect.  For example, go to the end of line
> 3376 of dispnew.c and type RET: point goes to column zero, not column
> 2.  If you convert "#if" to #ifdef", the behavior becomes correct.
> Same problem exists with #elif.
>
> Doesn't tree-sitter grammar consider #if and #elif preprocessor nodes?

It seems we are at the mercy of this[0] issue here, in all the cases
you've described.

>
> Also, there's a problem with #define in the middle of code.  For
> example, here:
>
>   /* Put a `display' property on the string for the captions to display,
>      put a `menu_item' property on tab-bar items with a value that
>      is the index of the item in F's tab-bar item vector.  */
>   for (i = 0; i < f->n_tab_bar_items; ++i)
>     {
> #define PROP(IDX) \
>   AREF (f->tab_bar_items, i * TAB_BAR_ITEM_NSLOTS + (IDX))
>
>       caption = Fcopy_sequence (PROP (TAB_BAR_ITEM_CAPTION));  <<<<<<<<<<
>
>       /* Put a `display' text property on the string for the caption to
> 	 display.  Put a `menu-item' property on the string that gives
> 	 the start of this item's properties in the tab-bar items
> 	 vector.  */
>       AUTO_LIST2 (props, Qmenu_item, make_fixnum (i * TAB_BAR_ITEM_NSLOTS));
>
>       Fadd_text_properties (make_fixnum (0), make_fixnum (SCHARS (caption)),
> 			    props, caption);
>
>       f->desired_tab_bar_string =
> 	concat2 (f->desired_tab_bar_string, caption);
>
> #undef PROP <<<<<<<<<<<<<<<<<<<<
>     }
>
> Typing RET at the end of the two marked lines goes to column zero
> instead of the expected non-zero column.  So it sounds like #define
> and #undef are also not handled correctly yet.

Yeah, luckily it indents correctly after you start to type.  I'll try to
see if I can make some specific handling for this.

Theo


[0]: https://github.com/tree-sitter/tree-sitter-c/issues/97

Message #23 received at 61558 <at> debbugs.gnu.org (full text, mbox):

From: Theodor Thornhill <theo <at> thornhill.no>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 61558 <at> debbugs.gnu.org
Subject: Re: bug#61558: 29.0.60; Indentation with c-ts-mode doesn't work in
 code guarded by #ifdef..#endif
Date: Sat, 18 Feb 2023 22:30:06 +0100

[Message part 1 (text/plain, inline)]

Theodor Thornhill <theo <at> thornhill.no> writes:

> Eli Zaretskii <eliz <at> gnu.org> writes:
>
>>> From: Theodor Thornhill <theo <at> thornhill.no>
>>> Cc: 61558 <at> debbugs.gnu.org
>>> Date: Fri, 17 Feb 2023 20:49:23 +0100
>>> 
>>> Can you test this patch?  It seems to work for me, but I'm no C expert.
>>
>> Thanks, this is much better.  However, the handling of #if (as opposed
>> to #ifdef) is still incorrect.  For example, go to the end of line
>> 3376 of dispnew.c and type RET: point goes to column zero, not column
>> 2.  If you convert "#if" to #ifdef", the behavior becomes correct.
>> Same problem exists with #elif.
>>
>> Doesn't tree-sitter grammar consider #if and #elif preprocessor nodes?
>
> It seems we are at the mercy of this[0] issue here, in all the cases
> you've described.
>
>>
>> Also, there's a problem with #define in the middle of code.  For
>> example, here:
>>
>>   /* Put a `display' property on the string for the captions to display,
>>      put a `menu_item' property on tab-bar items with a value that
>>      is the index of the item in F's tab-bar item vector.  */
>>   for (i = 0; i < f->n_tab_bar_items; ++i)
>>     {
>> #define PROP(IDX) \
>>   AREF (f->tab_bar_items, i * TAB_BAR_ITEM_NSLOTS + (IDX))
>>
>>       caption = Fcopy_sequence (PROP (TAB_BAR_ITEM_CAPTION));  <<<<<<<<<<
>>
>>       /* Put a `display' text property on the string for the caption to
>> 	 display.  Put a `menu-item' property on the string that gives
>> 	 the start of this item's properties in the tab-bar items
>> 	 vector.  */
>>       AUTO_LIST2 (props, Qmenu_item, make_fixnum (i * TAB_BAR_ITEM_NSLOTS));
>>
>>       Fadd_text_properties (make_fixnum (0), make_fixnum (SCHARS (caption)),
>> 			    props, caption);
>>
>>       f->desired_tab_bar_string =
>> 	concat2 (f->desired_tab_bar_string, caption);
>>
>> #undef PROP <<<<<<<<<<<<<<<<<<<<
>>     }
>>
>> Typing RET at the end of the two marked lines goes to column zero
>> instead of the expected non-zero column.  So it sounds like #define
>> and #undef are also not handled correctly yet.
>
> Yeah, luckily it indents correctly after you start to type.  I'll try to
> see if I can make some specific handling for this.
>
> Theo
>

Scratch that.  Can you test this for me?  I think I got it.

Theo

[0001-Cleanup-preproc-indent-for-c-ts-mode.patch (text/x-patch, attachment)]

[Message part 3 (text/plain, inline)]

>
> [0]: https://github.com/tree-sitter/tree-sitter-c/issues/97

Message #26 received at 61558 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Theodor Thornhill <theo <at> thornhill.no>
Cc: 61558 <at> debbugs.gnu.org
Subject: Re: bug#61558: 29.0.60; Indentation with c-ts-mode doesn't work in
 code guarded by #ifdef..#endif
Date: Sun, 19 Feb 2023 09:42:19 +0200

> From: Theodor Thornhill <theo <at> thornhill.no>
> Cc: 61558 <at> debbugs.gnu.org
> Date: Sat, 18 Feb 2023 22:30:06 +0100
> 
> >> Typing RET at the end of the two marked lines goes to column zero
> >> instead of the expected non-zero column.  So it sounds like #define
> >> and #undef are also not handled correctly yet.
> >
> > Yeah, luckily it indents correctly after you start to type.  I'll try to
> > see if I can make some specific handling for this.
> >
> > Theo
> >
> 
> Scratch that.  Can you test this for me?  I think I got it.

Thanks, this seems to work better.  Some problems still remain,
though.

Line 807 of dispnew.c:

#if defined (HAVE_WINDOW_SYSTEM) && ! defined (HAVE_EXT_TOOL_BAR)
  /* Clear the matrix of the tool-bar window, if any.  */
  if (WINDOWP (f->tool_bar_window))   <<<<<<<<<<<<<<<<<<
    clear_glyph_matrix (XWINDOW (f->tool_bar_window)->current_matrix);
#endif

Type RET at the end, then type '{' and RET.  The '{' gets indented
correctly, but there's no additional two-column indent after RET that
follows '{'.

RET after preprocessor directives outside of functions indents by 2
columns.  For example, here:

#if 0
/* Swap glyphs between two glyph rows A and B.  This exchanges glyph
   contents, i.e. glyph structure contents are exchanged between A and
   B without changing glyph pointers in A and B.  */

If you type RET after "#if 0", point goes to column 2, not zero.
Interestingly, if you type RET at the end of the last line of the
following comment, point goes to column zero, as expected.

Line 1357 of dispnew.c:

static void
free_glyph_pool (struct glyph_pool *pool)
{
  if (pool)
    {
#if defined GLYPH_DEBUG && defined ENABLE_CHECKING  <<<<<<<<<<<<<<<
      /* More freed than allocated?  */
      --glyph_pool_count;
      eassert (glyph_pool_count >= 0);
#endif
      xfree (pool->glyphs);
      xfree (pool);
    }
}

Type RET at the end of the indicated line: point goes to column 6, as
expected.  But if you then type "{ RET", point gets indented by 4
columns, not by 2.  And even typing a semi-colon afterwards doesn't
fix the indentation.

Similarly here:

static void
adjust_frame_glyphs_for_window_redisplay (struct frame *f)
{
  eassert (FRAME_WINDOW_P (f) && FRAME_LIVE_P (f));

  /* Allocate/reallocate window matrices.  */
  allocate_matrices_for_window_redisplay (XWINDOW (FRAME_ROOT_WINDOW (f)));

#if defined (HAVE_X_WINDOWS) && ! defined (USE_X_TOOLKIT) && ! defined (USE_GTK)
  /* Allocate/ reallocate matrices of the dummy window used to display
     the menu bar under X when no X toolkit support is available.  */
  {  <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
    /* Allocate a dummy window if not already done.  */
    struct window *w;
    if (NILP (f->menu_bar_window))

The indicated line is 2166 in dispnew.c.  If you type RET there, point
will be indented to column 6, not 4 as expected.  And if you type RET
at the end of the following comment line, not only will point be
over-indented, but the comment itself will also be reindented
incorrectly.

Anyway, this works much better than the original code, and I saw no
regressions, so I think you should install this.  Perhaps consider
adding comments explaining any tricky parts of handling this, so that
future hackers will know what to do and what to avoid.  Bonus points
for adding tests for this, so that we don't easily regress in the
future.

Thanks!

Message #29 received at 61558 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Theodor Thornhill <theo <at> thornhill.no>
Cc: 61558 <at> debbugs.gnu.org
Subject: Re: bug#61558: 29.0.60; Indentation with c-ts-mode doesn't work in
 code guarded by #ifdef..#endif
Date: Sun, 19 Feb 2023 09:43:49 +0200

> From: Theodor Thornhill <theo <at> thornhill.no>
> Cc: 61558 <at> debbugs.gnu.org
> Date: Sat, 18 Feb 2023 22:11:49 +0100
> 
> > Doesn't tree-sitter grammar consider #if and #elif preprocessor nodes?
> 
> It seems we are at the mercy of this[0] issue here, in all the cases
> you've described.

That issue is about #include, not #if.  Is that related?

Message #32 received at 61558 <at> debbugs.gnu.org (full text, mbox):

From: Theodor Thornhill <theo <at> thornhill.no>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 61558 <at> debbugs.gnu.org
Subject: Re: bug#61558: 29.0.60; Indentation with c-ts-mode doesn't work in code guarded by #ifdef..#endif
Date: Sun, 19 Feb 2023 09:01:01 +0100

On 19 February 2023 08:43:49 CET, Eli Zaretskii <eliz <at> gnu.org> wrote:
>> From: Theodor Thornhill <theo <at> thornhill.no>
>> Cc: 61558 <at> debbugs.gnu.org
>> Date: Sat, 18 Feb 2023 22:11:49 +0100
>> 
>> > Doesn't tree-sitter grammar consider #if and #elif preprocessor nodes?
>> 
>> It seems we are at the mercy of this[0] issue here, in all the cases
>> you've described.
>
>That issue is about #include, not #if.  Is that related?

Yeah, because all preprocs add these \n nodes that are a bit hard to account for at they are anonymous, and not declared as a legal node in the grammar. I think they are just there because some tokenizing bug.

Message #35 received at 61558 <at> debbugs.gnu.org (full text, mbox):

From: Theodor Thornhill <theo <at> thornhill.no>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 61558 <at> debbugs.gnu.org
Subject: Re: bug#61558: 29.0.60; Indentation with c-ts-mode doesn't work in
 code guarded by #ifdef..#endif
Date: Sun, 19 Feb 2023 10:20:50 +0100

Eli Zaretskii <eliz <at> gnu.org> writes:

>> From: Theodor Thornhill <theo <at> thornhill.no>
>> Cc: 61558 <at> debbugs.gnu.org
>> Date: Sat, 18 Feb 2023 22:30:06 +0100
>> 
>> >> Typing RET at the end of the two marked lines goes to column zero
>> >> instead of the expected non-zero column.  So it sounds like #define
>> >> and #undef are also not handled correctly yet.
>> >
>> > Yeah, luckily it indents correctly after you start to type.  I'll try to
>> > see if I can make some specific handling for this.
>> >
>> > Theo
>> >
>> 
>> Scratch that.  Can you test this for me?  I think I got it.
>
> Thanks, this seems to work better.  Some problems still remain,
> though.
>
> Line 807 of dispnew.c:
>
> #if defined (HAVE_WINDOW_SYSTEM) && ! defined (HAVE_EXT_TOOL_BAR)
>   /* Clear the matrix of the tool-bar window, if any.  */
>   if (WINDOWP (f->tool_bar_window))   <<<<<<<<<<<<<<<<<<
>     clear_glyph_matrix (XWINDOW (f->tool_bar_window)->current_matrix);
> #endif
>
> Type RET at the end, then type '{' and RET.  The '{' gets indented
> correctly, but there's no additional two-column indent after RET that
> follows '{'.

This is due to how 'c-ts-common-statement-offset' works, which seems to
assume balanced pairs.  I think this is "unrelated" to this bug.

>
> RET after preprocessor directives outside of functions indents by 2
> columns.  For example, here:
>
> #if 0
> /* Swap glyphs between two glyph rows A and B.  This exchanges glyph
>    contents, i.e. glyph structure contents are exchanged between A and
>    B without changing glyph pointers in A and B.  */
>
> If you type RET after "#if 0", point goes to column 2, not zero.
> Interestingly, if you type RET at the end of the last line of the
> following comment, point goes to column zero, as expected.

This one I'll fix.

>
> Line 1357 of dispnew.c:
>
> static void
> free_glyph_pool (struct glyph_pool *pool)
> {
>   if (pool)
>     {
> #if defined GLYPH_DEBUG && defined ENABLE_CHECKING  <<<<<<<<<<<<<<<
>       /* More freed than allocated?  */
>       --glyph_pool_count;
>       eassert (glyph_pool_count >= 0);
> #endif
>       xfree (pool->glyphs);
>       xfree (pool);
>     }
> }
>
> Type RET at the end of the indicated line: point goes to column 6, as
> expected.  But if you then type "{ RET", point gets indented by 4
> columns, not by 2.  And even typing a semi-colon afterwards doesn't
> fix the indentation.
>
> Similarly here:
>
> static void
> adjust_frame_glyphs_for_window_redisplay (struct frame *f)
> {
>   eassert (FRAME_WINDOW_P (f) && FRAME_LIVE_P (f));
>
>   /* Allocate/reallocate window matrices.  */
>   allocate_matrices_for_window_redisplay (XWINDOW (FRAME_ROOT_WINDOW (f)));
>
> #if defined (HAVE_X_WINDOWS) && ! defined (USE_X_TOOLKIT) && ! defined (USE_GTK)
>   /* Allocate/ reallocate matrices of the dummy window used to display
>      the menu bar under X when no X toolkit support is available.  */
>   {  <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
>     /* Allocate a dummy window if not already done.  */
>     struct window *w;
>     if (NILP (f->menu_bar_window))
>
> The indicated line is 2166 in dispnew.c.  If you type RET there, point
> will be indented to column 6, not 4 as expected.  And if you type RET
> at the end of the following comment line, not only will point be
> over-indented, but the comment itself will also be reindented
> incorrectly.
>
> Anyway, this works much better than the original code, and I saw no
> regressions, so I think you should install this.  Perhaps consider
> adding comments explaining any tricky parts of handling this, so that
> future hackers will know what to do and what to avoid.  Bonus points
> for adding tests for this, so that we don't easily regress in the
> future.
>

Great! Will do :-)

> Thanks!

No problem!

Message #38 received at 61558 <at> debbugs.gnu.org (full text, mbox):

From: Yuan Fu <casouri <at> gmail.com>
To: Theodor Thornhill <theo <at> thornhill.no>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 61558 <at> debbugs.gnu.org
Subject: Re: bug#61558: 29.0.60; Indentation with c-ts-mode doesn't work in 
 code guarded by #ifdef..#endif
Date: Fri, 24 Feb 2023 20:30:02 -0800

Theodor Thornhill <theo <at> thornhill.no> writes:

> Eli Zaretskii <eliz <at> gnu.org> writes:
>
>>> From: Theodor Thornhill <theo <at> thornhill.no>
>>> Cc: 61558 <at> debbugs.gnu.org
>>> Date: Sat, 18 Feb 2023 22:30:06 +0100
>>> 
>>> >> Typing RET at the end of the two marked lines goes to column zero
>>> >> instead of the expected non-zero column.  So it sounds like #define
>>> >> and #undef are also not handled correctly yet.
>>> >
>>> > Yeah, luckily it indents correctly after you start to type.  I'll try to
>>> > see if I can make some specific handling for this.
>>> >
>>> > Theo
>>> >
>>> 
>>> Scratch that.  Can you test this for me?  I think I got it.
>>
>> Thanks, this seems to work better.  Some problems still remain,
>> though.
>>
>> Line 807 of dispnew.c:
>>
>> #if defined (HAVE_WINDOW_SYSTEM) && ! defined (HAVE_EXT_TOOL_BAR)
>>   /* Clear the matrix of the tool-bar window, if any.  */
>>   if (WINDOWP (f->tool_bar_window))   <<<<<<<<<<<<<<<<<<
>>     clear_glyph_matrix (XWINDOW (f->tool_bar_window)->current_matrix);
>> #endif
>>
>> Type RET at the end, then type '{' and RET.  The '{' gets indented
>> correctly, but there's no additional two-column indent after RET that
>> follows '{'.
>
> This is due to how 'c-ts-common-statement-offset' works, which seems to
> assume balanced pairs.  I think this is "unrelated" to this bug.
>
>>
>> RET after preprocessor directives outside of functions indents by 2
>> columns.  For example, here:
>>
>> #if 0
>> /* Swap glyphs between two glyph rows A and B.  This exchanges glyph
>>    contents, i.e. glyph structure contents are exchanged between A and
>>    B without changing glyph pointers in A and B.  */
>>
>> If you type RET after "#if 0", point goes to column 2, not zero.
>> Interestingly, if you type RET at the end of the last line of the
>> following comment, point goes to column zero, as expected.
>
> This one I'll fix.
>
>>
>> Line 1357 of dispnew.c:
>>
>> static void
>> free_glyph_pool (struct glyph_pool *pool)
>> {
>>   if (pool)
>>     {
>> #if defined GLYPH_DEBUG && defined ENABLE_CHECKING  <<<<<<<<<<<<<<<
>>       /* More freed than allocated?  */
>>       --glyph_pool_count;
>>       eassert (glyph_pool_count >= 0);
>> #endif
>>       xfree (pool->glyphs);
>>       xfree (pool);
>>     }
>> }
>>
>> Type RET at the end of the indicated line: point goes to column 6, as
>> expected.  But if you then type "{ RET", point gets indented by 4
>> columns, not by 2.  And even typing a semi-colon afterwards doesn't
>> fix the indentation.
>>
>> Similarly here:
>>
>> static void
>> adjust_frame_glyphs_for_window_redisplay (struct frame *f)
>> {
>>   eassert (FRAME_WINDOW_P (f) && FRAME_LIVE_P (f));
>>
>>   /* Allocate/reallocate window matrices.  */
>>   allocate_matrices_for_window_redisplay (XWINDOW (FRAME_ROOT_WINDOW (f)));
>>
>> #if defined (HAVE_X_WINDOWS) && ! defined (USE_X_TOOLKIT) && ! defined (USE_GTK)
>>   /* Allocate/ reallocate matrices of the dummy window used to display
>>      the menu bar under X when no X toolkit support is available.  */
>>   {  <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
>>     /* Allocate a dummy window if not already done.  */
>>     struct window *w;
>>     if (NILP (f->menu_bar_window))
>>
>> The indicated line is 2166 in dispnew.c.  If you type RET there, point
>> will be indented to column 6, not 4 as expected.  And if you type RET
>> at the end of the following comment line, not only will point be
>> over-indented, but the comment itself will also be reindented
>> incorrectly.
>>
>> Anyway, this works much better than the original code, and I saw no
>> regressions, so I think you should install this.  Perhaps consider
>> adding comments explaining any tricky parts of handling this, so that
>> future hackers will know what to do and what to avoid.  Bonus points
>> for adding tests for this, so that we don't easily regress in the
>> future.
>>
>
> Great! Will do :-)
>
>> Thanks!
>
> No problem!


Sorry for joining late, I just added some change to support "indent
according to previous sibling" requested by someone on emacs-devel, and
noticed this change. IIUC, the goal is to indent whatever inside a
preproc directive as if the directive doesn’t exist, right? If that is
true, we should be fine by just using c-ts-common-statement-offset. Am I
missing something?

Statements inside labels are indented similarly, and this is the rule
used for them:

((parent-is "labeled_statement") point-min c-ts-common-statement-offset)

I tried to rewrite the current rule for preproc in the similar fasion,
ie, from

((n-p-gp nil "preproc" "translation_unit") point-min 0)
((n-p-gp nil "\n" "preproc") great-grand-parent c-ts-mode--preproc-offset)
((parent-is "preproc") grand-parent c-ts-mode-indent-offset)

to

((n-p-gp nil "\n" "preproc") point-min c-ts-common-statement-offset)
((parent-is "preproc") point-min c-ts-common-statement-offset)

and the test can pass.

Yuan

Message #41 received at 61558 <at> debbugs.gnu.org (full text, mbox):

From: Theodor Thornhill <theo <at> thornhill.no>
To: Yuan Fu <casouri <at> gmail.com>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 61558 <at> debbugs.gnu.org
Subject: Re: bug#61558: 29.0.60; Indentation with c-ts-mode doesn't work in  code guarded by #ifdef..#endif
Date: Sat, 25 Feb 2023 07:37:55 +0100

On 25 February 2023 05:30:02 CET, Yuan Fu <casouri <at> gmail.com> wrote:
>
>Theodor Thornhill <theo <at> thornhill.no> writes:
>
>> Eli Zaretskii <eliz <at> gnu.org> writes:
>>
>>>> From: Theodor Thornhill <theo <at> thornhill.no>
>>>> Cc: 61558 <at> debbugs.gnu.org
>>>> Date: Sat, 18 Feb 2023 22:30:06 +0100
>>>> 
>>>> >> Typing RET at the end of the two marked lines goes to column zero
>>>> >> instead of the expected non-zero column.  So it sounds like #define
>>>> >> and #undef are also not handled correctly yet.
>>>> >
>>>> > Yeah, luckily it indents correctly after you start to type.  I'll try to
>>>> > see if I can make some specific handling for this.
>>>> >
>>>> > Theo
>>>> >
>>>> 
>>>> Scratch that.  Can you test this for me?  I think I got it.
>>>
>>> Thanks, this seems to work better.  Some problems still remain,
>>> though.
>>>
>>> Line 807 of dispnew.c:
>>>
>>> #if defined (HAVE_WINDOW_SYSTEM) && ! defined (HAVE_EXT_TOOL_BAR)
>>>   /* Clear the matrix of the tool-bar window, if any.  */
>>>   if (WINDOWP (f->tool_bar_window))   <<<<<<<<<<<<<<<<<<
>>>     clear_glyph_matrix (XWINDOW (f->tool_bar_window)->current_matrix);
>>> #endif
>>>
>>> Type RET at the end, then type '{' and RET.  The '{' gets indented
>>> correctly, but there's no additional two-column indent after RET that
>>> follows '{'.
>>
>> This is due to how 'c-ts-common-statement-offset' works, which seems to
>> assume balanced pairs.  I think this is "unrelated" to this bug.
>>
>>>
>>> RET after preprocessor directives outside of functions indents by 2
>>> columns.  For example, here:
>>>
>>> #if 0
>>> /* Swap glyphs between two glyph rows A and B.  This exchanges glyph
>>>    contents, i.e. glyph structure contents are exchanged between A and
>>>    B without changing glyph pointers in A and B.  */
>>>
>>> If you type RET after "#if 0", point goes to column 2, not zero.
>>> Interestingly, if you type RET at the end of the last line of the
>>> following comment, point goes to column zero, as expected.
>>
>> This one I'll fix.
>>
>>>
>>> Line 1357 of dispnew.c:
>>>
>>> static void
>>> free_glyph_pool (struct glyph_pool *pool)
>>> {
>>>   if (pool)
>>>     {
>>> #if defined GLYPH_DEBUG && defined ENABLE_CHECKING  <<<<<<<<<<<<<<<
>>>       /* More freed than allocated?  */
>>>       --glyph_pool_count;
>>>       eassert (glyph_pool_count >= 0);
>>> #endif
>>>       xfree (pool->glyphs);
>>>       xfree (pool);
>>>     }
>>> }
>>>
>>> Type RET at the end of the indicated line: point goes to column 6, as
>>> expected.  But if you then type "{ RET", point gets indented by 4
>>> columns, not by 2.  And even typing a semi-colon afterwards doesn't
>>> fix the indentation.
>>>
>>> Similarly here:
>>>
>>> static void
>>> adjust_frame_glyphs_for_window_redisplay (struct frame *f)
>>> {
>>>   eassert (FRAME_WINDOW_P (f) && FRAME_LIVE_P (f));
>>>
>>>   /* Allocate/reallocate window matrices.  */
>>>   allocate_matrices_for_window_redisplay (XWINDOW (FRAME_ROOT_WINDOW (f)));
>>>
>>> #if defined (HAVE_X_WINDOWS) && ! defined (USE_X_TOOLKIT) && ! defined (USE_GTK)
>>>   /* Allocate/ reallocate matrices of the dummy window used to display
>>>      the menu bar under X when no X toolkit support is available.  */
>>>   {  <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
>>>     /* Allocate a dummy window if not already done.  */
>>>     struct window *w;
>>>     if (NILP (f->menu_bar_window))
>>>
>>> The indicated line is 2166 in dispnew.c.  If you type RET there, point
>>> will be indented to column 6, not 4 as expected.  And if you type RET
>>> at the end of the following comment line, not only will point be
>>> over-indented, but the comment itself will also be reindented
>>> incorrectly.
>>>
>>> Anyway, this works much better than the original code, and I saw no
>>> regressions, so I think you should install this.  Perhaps consider
>>> adding comments explaining any tricky parts of handling this, so that
>>> future hackers will know what to do and what to avoid.  Bonus points
>>> for adding tests for this, so that we don't easily regress in the
>>> future.
>>>
>>
>> Great! Will do :-)
>>
>>> Thanks!
>>
>> No problem!
>
>
>Sorry for joining late, I just added some change to support "indent
>according to previous sibling" requested by someone on emacs-devel, and
>noticed this change. IIUC, the goal is to indent whatever inside a
>preproc directive as if the directive doesn’t exist, right? If that is
>true, we should be fine by just using c-ts-common-statement-offset. Am I
>missing something?
>
>Statements inside labels are indented similarly, and this is the rule
>used for them:
>
>((parent-is "labeled_statement") point-min c-ts-common-statement-offset)
>
>I tried to rewrite the current rule for preproc in the similar fasion,
>ie, from
>
>((n-p-gp nil "preproc" "translation_unit") point-min 0)
>((n-p-gp nil "\n" "preproc") great-grand-parent c-ts-mode--preproc-offset)
>((parent-is "preproc") grand-parent c-ts-mode-indent-offset)
>
>to
>
>((n-p-gp nil "\n" "preproc") point-min c-ts-common-statement-offset)
>((parent-is "preproc") point-min c-ts-common-statement-offset)
>
>and the test can pass.
>
>Yuan


I have no strong opinions on this, but imo that function is pretty heavy at this point, and the rules i supplied are simple enough. C-ts-common-strtement-offset is an engine of its own and pretty complex by now, don't you think?

Message #44 received at 61558 <at> debbugs.gnu.org (full text, mbox):

From: Yuan Fu <casouri <at> gmail.com>
To: Theodor Thornhill <theo <at> thornhill.no>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 61558 <at> debbugs.gnu.org
Subject: Re: bug#61558: 29.0.60; Indentation with c-ts-mode doesn't work in 
 code guarded by #ifdef..#endif
Date: Sun, 26 Feb 2023 00:49:33 -0800

> On Feb 24, 2023, at 10:37 PM, Theodor Thornhill <theo <at> thornhill.no> wrote:
> 
> 
> 
> On 25 February 2023 05:30:02 CET, Yuan Fu <casouri <at> gmail.com> wrote:
>> 
>> Theodor Thornhill <theo <at> thornhill.no> writes:
>> 
>>> Eli Zaretskii <eliz <at> gnu.org> writes:
>>> 
>>>>> From: Theodor Thornhill <theo <at> thornhill.no>
>>>>> Cc: 61558 <at> debbugs.gnu.org
>>>>> Date: Sat, 18 Feb 2023 22:30:06 +0100
>>>>> 
>>>>>>> Typing RET at the end of the two marked lines goes to column zero
>>>>>>> instead of the expected non-zero column.  So it sounds like #define
>>>>>>> and #undef are also not handled correctly yet.
>>>>>> 
>>>>>> Yeah, luckily it indents correctly after you start to type.  I'll try to
>>>>>> see if I can make some specific handling for this.
>>>>>> 
>>>>>> Theo
>>>>>> 
>>>>> 
>>>>> Scratch that.  Can you test this for me?  I think I got it.
>>>> 
>>>> Thanks, this seems to work better.  Some problems still remain,
>>>> though.
>>>> 
>>>> Line 807 of dispnew.c:
>>>> 
>>>> #if defined (HAVE_WINDOW_SYSTEM) && ! defined (HAVE_EXT_TOOL_BAR)
>>>>  /* Clear the matrix of the tool-bar window, if any.  */
>>>>  if (WINDOWP (f->tool_bar_window))   <<<<<<<<<<<<<<<<<<
>>>>    clear_glyph_matrix (XWINDOW (f->tool_bar_window)->current_matrix);
>>>> #endif
>>>> 
>>>> Type RET at the end, then type '{' and RET.  The '{' gets indented
>>>> correctly, but there's no additional two-column indent after RET that
>>>> follows '{'.
>>> 
>>> This is due to how 'c-ts-common-statement-offset' works, which seems to
>>> assume balanced pairs.  I think this is "unrelated" to this bug.
>>> 
>>>> 
>>>> RET after preprocessor directives outside of functions indents by 2
>>>> columns.  For example, here:
>>>> 
>>>> #if 0
>>>> /* Swap glyphs between two glyph rows A and B.  This exchanges glyph
>>>>   contents, i.e. glyph structure contents are exchanged between A and
>>>>   B without changing glyph pointers in A and B.  */
>>>> 
>>>> If you type RET after "#if 0", point goes to column 2, not zero.
>>>> Interestingly, if you type RET at the end of the last line of the
>>>> following comment, point goes to column zero, as expected.
>>> 
>>> This one I'll fix.
>>> 
>>>> 
>>>> Line 1357 of dispnew.c:
>>>> 
>>>> static void
>>>> free_glyph_pool (struct glyph_pool *pool)
>>>> {
>>>>  if (pool)
>>>>    {
>>>> #if defined GLYPH_DEBUG && defined ENABLE_CHECKING  <<<<<<<<<<<<<<<
>>>>      /* More freed than allocated?  */
>>>>      --glyph_pool_count;
>>>>      eassert (glyph_pool_count >= 0);
>>>> #endif
>>>>      xfree (pool->glyphs);
>>>>      xfree (pool);
>>>>    }
>>>> }
>>>> 
>>>> Type RET at the end of the indicated line: point goes to column 6, as
>>>> expected.  But if you then type "{ RET", point gets indented by 4
>>>> columns, not by 2.  And even typing a semi-colon afterwards doesn't
>>>> fix the indentation.
>>>> 
>>>> Similarly here:
>>>> 
>>>> static void
>>>> adjust_frame_glyphs_for_window_redisplay (struct frame *f)
>>>> {
>>>>  eassert (FRAME_WINDOW_P (f) && FRAME_LIVE_P (f));
>>>> 
>>>>  /* Allocate/reallocate window matrices.  */
>>>>  allocate_matrices_for_window_redisplay (XWINDOW (FRAME_ROOT_WINDOW (f)));
>>>> 
>>>> #if defined (HAVE_X_WINDOWS) && ! defined (USE_X_TOOLKIT) && ! defined (USE_GTK)
>>>>  /* Allocate/ reallocate matrices of the dummy window used to display
>>>>     the menu bar under X when no X toolkit support is available.  */
>>>>  {  <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
>>>>    /* Allocate a dummy window if not already done.  */
>>>>    struct window *w;
>>>>    if (NILP (f->menu_bar_window))
>>>> 
>>>> The indicated line is 2166 in dispnew.c.  If you type RET there, point
>>>> will be indented to column 6, not 4 as expected.  And if you type RET
>>>> at the end of the following comment line, not only will point be
>>>> over-indented, but the comment itself will also be reindented
>>>> incorrectly.
>>>> 
>>>> Anyway, this works much better than the original code, and I saw no
>>>> regressions, so I think you should install this.  Perhaps consider
>>>> adding comments explaining any tricky parts of handling this, so that
>>>> future hackers will know what to do and what to avoid.  Bonus points
>>>> for adding tests for this, so that we don't easily regress in the
>>>> future.
>>>> 
>>> 
>>> Great! Will do :-)
>>> 
>>>> Thanks!
>>> 
>>> No problem!
>> 
>> 
>> Sorry for joining late, I just added some change to support "indent
>> according to previous sibling" requested by someone on emacs-devel, and
>> noticed this change. IIUC, the goal is to indent whatever inside a
>> preproc directive as if the directive doesn’t exist, right? If that is
>> true, we should be fine by just using c-ts-common-statement-offset. Am I
>> missing something?
>> 
>> Statements inside labels are indented similarly, and this is the rule
>> used for them:
>> 
>> ((parent-is "labeled_statement") point-min c-ts-common-statement-offset)
>> 
>> I tried to rewrite the current rule for preproc in the similar fasion,
>> ie, from
>> 
>> ((n-p-gp nil "preproc" "translation_unit") point-min 0)
>> ((n-p-gp nil "\n" "preproc") great-grand-parent c-ts-mode--preproc-offset)
>> ((parent-is "preproc") grand-parent c-ts-mode-indent-offset)
>> 
>> to
>> 
>> ((n-p-gp nil "\n" "preproc") point-min c-ts-common-statement-offset)
>> ((parent-is "preproc") point-min c-ts-common-statement-offset)
>> 
>> and the test can pass.
>> 
>> Yuan
> 
> 
> I have no strong opinions on this, but imo that function is pretty heavy at this point, and the rules i supplied are simple enough. C-ts-common-strtement-offset is an engine of its own and pretty complex by now, don't you think?
> 

Sure. As long you are satisfied, I’m fine with it. c-ts-common-statement-offset is indeed too heavy. I’m working to improve c-ts-common-statement-offset and make it more like parent-bol (so it’s lighter).

Yuan

Message #47 received at 61558 <at> debbugs.gnu.org (full text, mbox):

From: Theodor Thornhill <theo <at> thornhill.no>
To: Yuan Fu <casouri <at> gmail.com>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 61558 <at> debbugs.gnu.org
Subject: Re: bug#61558: 29.0.60; Indentation with c-ts-mode doesn't work in  code guarded by #ifdef..#endif
Date: Sun, 26 Feb 2023 11:30:44 +0100

On 26 February 2023 09:49:33 CET, Yuan Fu <casouri <at> gmail.com> wrote:
>
>
>> On Feb 24, 2023, at 10:37 PM, Theodor Thornhill <theo <at> thornhill.no> wrote:
>> 
>> 
>> 
>> On 25 February 2023 05:30:02 CET, Yuan Fu <casouri <at> gmail.com> wrote:
>>> 
>>> Theodor Thornhill <theo <at> thornhill.no> writes:
>>> 
>>>> Eli Zaretskii <eliz <at> gnu.org> writes:
>>>> 
>>>>>> From: Theodor Thornhill <theo <at> thornhill.no>
>>>>>> Cc: 61558 <at> debbugs.gnu.org
>>>>>> Date: Sat, 18 Feb 2023 22:30:06 +0100
>>>>>> 
>>>>>>>> Typing RET at the end of the two marked lines goes to column zero
>>>>>>>> instead of the expected non-zero column.  So it sounds like #define
>>>>>>>> and #undef are also not handled correctly yet.
>>>>>>> 
>>>>>>> Yeah, luckily it indents correctly after you start to type.  I'll try to
>>>>>>> see if I can make some specific handling for this.
>>>>>>> 
>>>>>>> Theo
>>>>>>> 
>>>>>> 
>>>>>> Scratch that.  Can you test this for me?  I think I got it.
>>>>> 
>>>>> Thanks, this seems to work better.  Some problems still remain,
>>>>> though.
>>>>> 
>>>>> Line 807 of dispnew.c:
>>>>> 
>>>>> #if defined (HAVE_WINDOW_SYSTEM) && ! defined (HAVE_EXT_TOOL_BAR)
>>>>>  /* Clear the matrix of the tool-bar window, if any.  */
>>>>>  if (WINDOWP (f->tool_bar_window))   <<<<<<<<<<<<<<<<<<
>>>>>    clear_glyph_matrix (XWINDOW (f->tool_bar_window)->current_matrix);
>>>>> #endif
>>>>> 
>>>>> Type RET at the end, then type '{' and RET.  The '{' gets indented
>>>>> correctly, but there's no additional two-column indent after RET that
>>>>> follows '{'.
>>>> 
>>>> This is due to how 'c-ts-common-statement-offset' works, which seems to
>>>> assume balanced pairs.  I think this is "unrelated" to this bug.
>>>> 
>>>>> 
>>>>> RET after preprocessor directives outside of functions indents by 2
>>>>> columns.  For example, here:
>>>>> 
>>>>> #if 0
>>>>> /* Swap glyphs between two glyph rows A and B.  This exchanges glyph
>>>>>   contents, i.e. glyph structure contents are exchanged between A and
>>>>>   B without changing glyph pointers in A and B.  */
>>>>> 
>>>>> If you type RET after "#if 0", point goes to column 2, not zero.
>>>>> Interestingly, if you type RET at the end of the last line of the
>>>>> following comment, point goes to column zero, as expected.
>>>> 
>>>> This one I'll fix.
>>>> 
>>>>> 
>>>>> Line 1357 of dispnew.c:
>>>>> 
>>>>> static void
>>>>> free_glyph_pool (struct glyph_pool *pool)
>>>>> {
>>>>>  if (pool)
>>>>>    {
>>>>> #if defined GLYPH_DEBUG && defined ENABLE_CHECKING  <<<<<<<<<<<<<<<
>>>>>      /* More freed than allocated?  */
>>>>>      --glyph_pool_count;
>>>>>      eassert (glyph_pool_count >= 0);
>>>>> #endif
>>>>>      xfree (pool->glyphs);
>>>>>      xfree (pool);
>>>>>    }
>>>>> }
>>>>> 
>>>>> Type RET at the end of the indicated line: point goes to column 6, as
>>>>> expected.  But if you then type "{ RET", point gets indented by 4
>>>>> columns, not by 2.  And even typing a semi-colon afterwards doesn't
>>>>> fix the indentation.
>>>>> 
>>>>> Similarly here:
>>>>> 
>>>>> static void
>>>>> adjust_frame_glyphs_for_window_redisplay (struct frame *f)
>>>>> {
>>>>>  eassert (FRAME_WINDOW_P (f) && FRAME_LIVE_P (f));
>>>>> 
>>>>>  /* Allocate/reallocate window matrices.  */
>>>>>  allocate_matrices_for_window_redisplay (XWINDOW (FRAME_ROOT_WINDOW (f)));
>>>>> 
>>>>> #if defined (HAVE_X_WINDOWS) && ! defined (USE_X_TOOLKIT) && ! defined (USE_GTK)
>>>>>  /* Allocate/ reallocate matrices of the dummy window used to display
>>>>>     the menu bar under X when no X toolkit support is available.  */
>>>>>  {  <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
>>>>>    /* Allocate a dummy window if not already done.  */
>>>>>    struct window *w;
>>>>>    if (NILP (f->menu_bar_window))
>>>>> 
>>>>> The indicated line is 2166 in dispnew.c.  If you type RET there, point
>>>>> will be indented to column 6, not 4 as expected.  And if you type RET
>>>>> at the end of the following comment line, not only will point be
>>>>> over-indented, but the comment itself will also be reindented
>>>>> incorrectly.
>>>>> 
>>>>> Anyway, this works much better than the original code, and I saw no
>>>>> regressions, so I think you should install this.  Perhaps consider
>>>>> adding comments explaining any tricky parts of handling this, so that
>>>>> future hackers will know what to do and what to avoid.  Bonus points
>>>>> for adding tests for this, so that we don't easily regress in the
>>>>> future.
>>>>> 
>>>> 
>>>> Great! Will do :-)
>>>> 
>>>>> Thanks!
>>>> 
>>>> No problem!
>>> 
>>> 
>>> Sorry for joining late, I just added some change to support "indent
>>> according to previous sibling" requested by someone on emacs-devel, and
>>> noticed this change. IIUC, the goal is to indent whatever inside a
>>> preproc directive as if the directive doesn’t exist, right? If that is
>>> true, we should be fine by just using c-ts-common-statement-offset. Am I
>>> missing something?
>>> 
>>> Statements inside labels are indented similarly, and this is the rule
>>> used for them:
>>> 
>>> ((parent-is "labeled_statement") point-min c-ts-common-statement-offset)
>>> 
>>> I tried to rewrite the current rule for preproc in the similar fasion,
>>> ie, from
>>> 
>>> ((n-p-gp nil "preproc" "translation_unit") point-min 0)
>>> ((n-p-gp nil "\n" "preproc") great-grand-parent c-ts-mode--preproc-offset)
>>> ((parent-is "preproc") grand-parent c-ts-mode-indent-offset)
>>> 
>>> to
>>> 
>>> ((n-p-gp nil "\n" "preproc") point-min c-ts-common-statement-offset)
>>> ((parent-is "preproc") point-min c-ts-common-statement-offset)
>>> 
>>> and the test can pass.
>>> 
>>> Yuan
>> 
>> 
>> I have no strong opinions on this, but imo that function is pretty heavy at this point, and the rules i supplied are simple enough. C-ts-common-strtement-offset is an engine of its own and pretty complex by now, don't you think?
>> 
>
>Sure. As long you are satisfied, I’m fine with it. c-ts-common-statement-offset is indeed too heavy. I’m working to improve c-ts-common-statement-offset and make it more like parent-bol (so it’s lighter).
>
>Yuan
>

Nice!
I think we can revisit it if we need to. I think it works pretty well right now :)

Theo

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.