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

[Theodor Thornhill]

On 25 February 2023 05:30:02 CET, Yuan Fu <casouri@gmail.com> wrote:
>
>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


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?