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

[Yuan Fu]

> On Feb 24, 2023, at 10:37 PM, Theodor Thornhill <theo@thornhill.no> wrote:
> 
> 
> 
> 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?
> 

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