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

Previous Next

Full log

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.