Package: emacs;
Reported by: Eli Zaretskii <eliz <at> gnu.org>
Date: Thu, 16 Feb 2023 20:49:01 UTC
Severity: normal
Found in version 29.0.60
View this message in rfc822 format
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: 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?
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.