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

[Yuan Fu]

Theodor Thornhill <theo@thornhill.no> writes:

> Eli Zaretskii <eliz@gnu.org> writes:
>
>>> From: Theodor Thornhill <theo@thornhill.no>
>>> Cc: 61558@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