GNU bug report logs - #385
[PATCH] comment-indent doesn't respect comment-indent-function

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 385 in the body.
You can then email your comments to 385 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> emacsbugs.donarmstrong.com (full text, mbox):

From: "Christopher J. Madsen" <cjm <at> cjmweb.net>
To: bug-gnu-emacs <at> gnu.org
Subject: [PATCH] comment-indent doesn't respect comment-indent-function
Date: 11 Jun 2008 17:11:02 -0000

Please describe exactly what actions triggered the bug
and the precise symptoms of the bug:

It appears that comment-indent changed in 22.1.  It gained some code
to attempt to align the comment with those on surrounding lines.
Unfortunately, this made it impossible to do certain things with
comment-indent-function.

For example, I had a custom indent function that placed comments
immediately after a closing brace.  However, in Emacs 22, I'd see this:

  while (1) {
    while (2) {

    } # end 2 <-- this comment placed correctly
  }   # end 1 <-- this comment was aligned with the previous one

instead of this:

  while (1) {
    while (2) {

    } # end 2
  } # end 1 <-- here's where comment-indent-function placed it

On the other hand, I do like the idea of automatically aligning
comments.  I had code in my custom indent functions to do that, but it
would be nice if I didn't need to handle that in every indent
function.

I think what's needed is a way for comment-indent-function to
distinguish between "Here's where the comment goes, and that's final"
and "I suggest this position, but make it blend in with the
neighborhood".  Ideally, this would be backwards-compatible with older
versions of Emacs.

Here's a patch I came up with to provide that.  If
comment-indent-function sets comment-indent-fixed to non-nil, then the
return value will be used as-is.  Otherwise, it behaves like Emacs
22.2 did.

Perhaps the sense should be reversed, and it should always respect the
value of comment-indent-function unless told it's ok to adjust it.


*** orig/newcomment.el	Fri Mar 07 18:01:12 2008
--- new/newcomment.el	Wed Jun 11 11:13:24 2008
*************** (defvar comment-indent-function 'comment
*** 135,140 ****
--- 135,143 ----
  This function is called with no args with point at the beginning of
  the comment's starting delimiter and should return either the desired
  column indentation or nil.
+ The returned value may be adjusted by `comment-choose-indent'.
+ To prevent that, the function should set `comment-indent-fixed'
+ to a non-nil value.
  If nil is returned, indentation is delegated to `indent-according-to-mode'.")
  
  ;;;###autoload
*************** (defun comment-indent (&optional continu
*** 585,591 ****
      (beginning-of-line)
      (let* ((eolpos (line-end-position))
  	   (begpos (comment-search-forward eolpos t))
! 	   cpos indent)
        ;; An existing comment?
        (if begpos
  	  (progn
--- 588,594 ----
      (beginning-of-line)
      (let* ((eolpos (line-end-position))
  	   (begpos (comment-search-forward eolpos t))
! 	   cpos indent comment-indent-fixed)
        ;; An existing comment?
        (if begpos
  	  (progn
*************** (defun comment-indent (&optional continu
*** 622,636 ****
        (if (not indent)
  	  ;; comment-indent-function refuses: delegate to line-indent.
  	  (indent-according-to-mode)
! 	;; If the comment is at the right of code, adjust the indentation.
! 	(unless (save-excursion (skip-chars-backward " \t") (bolp))
!           (setq indent (comment-choose-indent indent)))
! 	;; Update INDENT to leave at least one space
! 	;; after other nonwhite text on the line.
! 	(save-excursion
! 	  (skip-chars-backward " \t")
! 	  (unless (bolp)
! 	    (setq indent (max indent (1+ (current-column))))))
  	;; If that's different from comment's current position, change it.
  	(unless (= (current-column) indent)
  	  (delete-region (point) (progn (skip-chars-backward " \t") (point)))
--- 625,640 ----
        (if (not indent)
  	  ;; comment-indent-function refuses: delegate to line-indent.
  	  (indent-according-to-mode)
!         (unless comment-indent-fixed
!           ;; If the comment is at the right of code, adjust the indentation.
!           (unless (save-excursion (skip-chars-backward " \t") (bolp))
!             (setq indent (comment-choose-indent indent)))
!           ;; Update INDENT to leave at least one space
!           ;; after other nonwhite text on the line.
!           (save-excursion
!             (skip-chars-backward " \t")
!             (unless (bolp)
!               (setq indent (max indent (1+ (current-column)))))))
  	;; If that's different from comment's current position, change it.
  	(unless (= (current-column) indent)
  	  (delete-region (point) (progn (skip-chars-backward " \t") (point)))


In GNU Emacs 22.2.1 (i386-mingw-nt5.1.2600)
 of 2008-03-26 on RELEASE
Windowing system distributor `Microsoft Corp.', version 5.1.2600
configured using `configure --with-gcc (3.4)'

-- 
Chris Madsen                                           cjm cjmweb.net
  --------------------  http://www.cjmweb.net  --------------------

Message #10 received at submit <at> emacsbugs.donarmstrong.com (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: "Christopher J. Madsen" <cjm <at> cjmweb.net>
Cc: 385 <at> debbugs.gnu.org, bug-gnu-emacs <at> gnu.org
Subject: Re: bug#385: [PATCH] comment-indent doesn't respect comment-indent-function
Date: Wed, 11 Jun 2008 14:04:57 -0400

> It appears that comment-indent changed in 22.1.  It gained some code

Indeed, and it changed further in 22.2.

> to attempt to align the comment with those on surrounding lines.
> Unfortunately, this made it impossible to do certain things with
> comment-indent-function.

> For example, I had a custom indent function that placed comments
> immediately after a closing brace.  However, in Emacs 22, I'd see this:

>   while (1) {
>     while (2) {

>     } # end 2 <-- this comment placed correctly
>   }   # end 1 <-- this comment was aligned with the previous one

> instead of this:

>   while (1) {
>     while (2) {

>     } # end 2
>   } # end 1 <-- here's where comment-indent-function placed it

I'm not sure I understand.  Are you saying that you don't want comments
to be aligned in that case?

If you need more control over the placement, rather than a variable
comment-indent-fixed, maybe we should just say that if
comment-indent-function returns a list of a single integer, it should be
taken as the indentation position and not second-guessed.  Or it could
return a cons cell (MIN . MAX) to say "anywhere between MIN and MAX".


        Stefan

Message #20 received at submit <at> emacsbugs.donarmstrong.com (full text, mbox):

From: "Christopher J. Madsen" <cjm <at> cjmweb.net>
To: "Stefan Monnier" <monnier <at> iro.umontreal.ca>
Cc: 385 <at> debbugs.gnu.org, bug-gnu-emacs <at> gnu.org
Subject: Re: bug#385: [PATCH] comment-indent doesn't respect 
 comment-indent-function
Date: Wed, 11 Jun 2008 13:59:01 -0500 (CDT)

On Wed, June 11, 2008 1:04 pm, Stefan Monnier wrote:
>> It appears that comment-indent changed in 22.1.  It gained some code

>> For example, I had a custom indent function that placed comments
>> immediately after a closing brace.  However, in Emacs 22, I'd see this:
>
>>   while (1) {
>>     while (2) {
>
>>     } # end 2 <-- this comment placed correctly
>>   }   # end 1 <-- this comment was aligned with the previous one
>
>> instead of this:
>
>>   while (1) {
>>     while (2) {
>
>>     } # end 2
>>   } # end 1 <-- here's where comment-indent-function placed it
>
> I'm not sure I understand.  Are you saying that you don't want comments
> to be aligned in that case?

Yes.  I want the comment one space after the closing brace.  Period.  In
Emacs 22, there's no way for the comment-indent-function to say "Put it
here and don't second guess me."

> If you need more control over the placement, rather than a variable
> comment-indent-fixed, maybe we should just say that if
> comment-indent-function returns a list of a single integer, it should be
> taken as the indentation position and not second-guessed.  Or it could
> return a cons cell (MIN . MAX) to say "anywhere between MIN and MAX".

I thought about something like that.  The problem is that current versions
of Emacs would have no idea what to do with a return value that's not an
integer.  I use a variety of Emacs versions on a number of machines.  The
indent function would have to check emacs-version and change the return
value accordingly.  That's always a mess.

The advantage of my approach is that you can use the same indent function
on any version of Emacs.  Older versions just won't pay any attention to
comment-indent-fixed.  Otherwise, I'd go with returning a list.

-- 
Chris Madsen                                           cjm cjmweb.net
   --------------------  http://www.cjmweb.net  --------------------

Message #30 received at 385 <at> emacsbugs.donarmstrong.com (full text, mbox):

From: Stefan Monnier <monnier <at> IRO.UMontreal.CA>
To: 385 <at> debbugs.gnu.org
Subject: Re: bug#385: [PATCH] comment-indent doesn't respect
Date: Fri, 13 Jun 2008 12:47:41 -0400

severity 385 minor
thanks

> I thought about something like that.  The problem is that current versions
> of Emacs would have no idea what to do with a return value that's not an
> integer.  I use a variety of Emacs versions on a number of machines.  The
> indent function would have to check emacs-version and change the return
> value accordingly.  That's always a mess.

I understand, but a variable like you suggests makes it impossible to
have the special indentation you want together with the auto-alignment
for other comment cases.

I don't want a half solution, just to make the transition easier.
You're trading off a minor short term gain again a long term loss.


        Stefan

View this message in rfc822 format

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: "Christopher J. Madsen" <cjm <at> cjmweb.net>
Cc: 385 <at> debbugs.gnu.org
Subject: bug#385: [PATCH] comment-indent doesn't respect comment-indent-function
Date: Mon, 29 Feb 2016 15:59:01 +1100

"Christopher J. Madsen" <cjm <at> cjmweb.net> writes:

> Please describe exactly what actions triggered the bug
> and the precise symptoms of the bug:
>
> It appears that comment-indent changed in 22.1.  It gained some code
> to attempt to align the comment with those on surrounding lines.
> Unfortunately, this made it impossible to do certain things with
> comment-indent-function.
>
> For example, I had a custom indent function that placed comments
> immediately after a closing brace.  However, in Emacs 22, I'd see this:
>
>   while (1) {
>     while (2) {
>
>     } # end 2 <-- this comment placed correctly
>   }   # end 1 <-- this comment was aligned with the previous one
>
> instead of this:
>
>   while (1) {
>     while (2) {
>
>     } # end 2
>   } # end 1 <-- here's where comment-indent-function placed it

Is this still an issue in Emacs 25?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Message #39 received at 385 <at> debbugs.gnu.org (full text, mbox):

From: "Christopher J. Madsen" <cjm <at> cjmweb.net>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 385 <at> debbugs.gnu.org
Subject: Re: bug#385: [PATCH] comment-indent doesn't respect
 comment-indent-function
Date: Wed, 2 Mar 2016 23:03:37 -0600

On 2/28/2016 10:59 PM, Lars Ingebrigtsen wrote:
> Is this still an issue in Emacs 25? 

Yes.  I built emacs-25.1.50.1 from 04289d1cd and nothing seems to have
changed.  The fundamental problem is that there's no way for a
comment-indent-function to say "Put the comment here, and I really mean
it."  In some situations, comment-indent will always second-guess the
comment-indent-function.  In particular, it insists on aligning the
comment with a comment on the line above even when that's not what I want.

To reproduce this, load this Perl code:

#! /usr/bin/perl

if (1) {
  if (2) {
    if (3) {
      4;
    } # end 3
  } # end 2
} # end 1

And set comment-indent-function to this function:

(defun cjm-perl-comment-indent ()
  (if (and (bolp) (not (eolp)))
      0                                 ;Existing comment at bol stays
there.
    (save-excursion
      ;; endcol is the minimum column number the comment can start at
      ;; and still leave one space after text already on the line
      (skip-chars-backward " \t")
      (let ((endcol (1+ (current-column))))
        (if (= 1 endcol)                ;Don't leave just one space
            (setq endcol 0))            ;at beginning of line
        (beginning-of-line)
        (cond
         ;; CASE 1: A comment following a solitary closing brace should
         ;; have only one space.
         ((looking-at "[ \t]*}[ \t]*\\($\\|#\\)")
          endcol)
         ;; CASE 2: Align with comment on previous line
         ;; unless that's more than 9 chars before comment-column,
         ;; and leave at least one space (unless starting at bol).
         ((and (= 0 (forward-line -1))
               (looking-at ".*[ \t]\\(#\\)")
               (progn
                 (goto-char (match-beginning 1))
                 (> 10 (- comment-column (current-column)))))
          (max (current-column) endcol))
         ;; CASE 3: indent at comment column except leave at least one
         ;; space (unless at bol)
         (t (max endcol comment-column))
         )))))

Put point on the "end 2" line, hit M-; and the comment will be moved to
align with the "end 3 " comment instead of staying where it was.  Repeat
on the "end 1" line and you'll have

    } # end 3
  }   # end 2
}     # end 1

instead of the original code.

(You don't actually need a comment-indent-function this complex to
reproduce the issue, but this is the actual function I use.)

Sorry for the delay in getting back to you.

-- 
Chris Madsen                                           cjm <at> cjmweb.net
  --------------------  http://www.cjmweb.net  --------------------

Message #42 received at 385 <at> debbugs.gnu.org (full text, mbox):

From: npostavs <at> users.sourceforge.net
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 385 <at> debbugs.gnu.org, "Christopher J. Madsen" <cjm <at> cjmweb.net>
Subject: Re: bug#385: [PATCH] comment-indent doesn't respect
 comment-indent-function
Date: Wed, 14 Jun 2017 00:33:06 -0400

[Message part 1 (text/plain, inline)]

tags 19740 + patch
block 19740 by 385
quit

Stefan Monnier <monnier <at> iro.umontreal.ca> writes:

> If you need more control over the placement, rather than a variable
> comment-indent-fixed, maybe we should just say that if
> comment-indent-function returns a list of a single integer, it should be
> taken as the indentation position and not second-guessed.  Or it could
> return a cons cell (MIN . MAX) to say "anywhere between MIN and MAX".

Here's a patch.  This seems to be a prerequisite to fix #19740.

Regarding incompatibility of new comment-indent-functions for old Emacs,
some simple advice on comment-choose-indent should easily do the trick.

[v1-0001-Allow-comment-indent-functions-to-specify-exact-i.patch (text/x-diff, inline)]

From cc9db0dbb5590ee909386078128e55c5ee24f319 Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs <at> gmail.com>
Date: Wed, 14 Jun 2017 00:08:15 -0400
Subject: [PATCH v1] Allow comment-indent-functions to specify exact
 indentation (Bug#385)

* lisp/newcomment.el (comment-choose-indent): Interpret a cons of two
integers as indicating a range of acceptable indentation.
(comment-indent): Don't apply `comment-inline-offset',
`comment-choose-indent' already does that.
(comment-indent-function):
* doc/emacs/programs.texi (Options for Comments): Document new
acceptable return values.
* etc/NEWS: Announce it.
---
 doc/emacs/programs.texi |  9 ++++++---
 etc/NEWS                |  4 ++++
 lisp/newcomment.el      | 35 ++++++++++++++++++-----------------
 3 files changed, 28 insertions(+), 20 deletions(-)

diff --git a/doc/emacs/programs.texi b/doc/emacs/programs.texi
index 222d1c2a4d..27ac0eb640 100644
--- a/doc/emacs/programs.texi
+++ b/doc/emacs/programs.texi
@@ -1146,9 +1146,12 @@ Options for Comments
 various major modes.  The function is called with no arguments, but with
 point at the beginning of the comment, or at the end of a line if a new
 comment is to be inserted.  It should return the column in which the
-comment ought to start.  For example, in Lisp mode, the indent hook
-function bases its decision on how many semicolons begin an existing
-comment, and on the code in the preceding lines.
+comment ought to start.  For example, the default hook function bases
+its decision on how many comment characters begin an existing comment.
+
+Emacs also tries to align comments on adjacent lines.  To override
+this, the function may return a cons of two (possibly equal) integers
+to indicate an acceptable range of indentation.
 
 @node Documentation
 @section Documentation Lookup
diff --git a/etc/NEWS b/etc/NEWS
index 7e955ad26d..2467e81fe3 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -377,6 +377,10 @@ display of raw bytes from octal to hex.
 ** You can now provide explicit field numbers in format specifiers.
 For example, '(format "%2$s %1$s" "X" "Y")' produces "Y X".
 
++++
+** 'comment-indent-function' values may now return a cons to specify a
+range of indentation.
+
 
 * Editing Changes in Emacs 26.1
 
diff --git a/lisp/newcomment.el b/lisp/newcomment.el
index 118549f421..8772b52376 100644
--- a/lisp/newcomment.el
+++ b/lisp/newcomment.el
@@ -142,9 +142,10 @@ (put 'comment-end 'safe-local-variable 'stringp)
 ;;;###autoload
 (defvar comment-indent-function 'comment-indent-default
   "Function to compute desired indentation for a comment.
-This function is called with no args with point at the beginning of
-the comment's starting delimiter and should return either the desired
-column indentation or nil.
+This function is called with no args with point at the beginning
+of the comment's starting delimiter and should return either the
+desired column indentation, a range of acceptable
+indentation (MIN . MAX), or nil.
 If nil is returned, indentation is delegated to `indent-according-to-mode'.")
 
 ;;;###autoload
@@ -649,13 +650,20 @@ (defun comment-choose-indent (&optional indent)
 - prefer INDENT (or `comment-column' if nil).
 Point is expected to be at the start of the comment."
   (unless indent (setq indent comment-column))
-  ;; Avoid moving comments past the fill-column.
-  (let ((max (+ (current-column)
-                (- (or comment-fill-column fill-column)
-                   (save-excursion (end-of-line) (current-column)))))
-        (other nil)
-        (min (save-excursion (skip-chars-backward " \t")
-                             (if (bolp) 0 (+ comment-inline-offset (current-column))))))
+  (let ((other nil)
+        min max)
+    (pcase indent
+      (`(,lo . ,hi) (setq min lo) (setq max hi)
+       (setq indent comment-column))
+      (_ ;; Avoid moving comments past the fill-column.
+       (setq max (+ (current-column)
+                    (- (or comment-fill-column fill-column)
+                       (save-excursion (end-of-line) (current-column)))))
+       (setq min (save-excursion
+                   (skip-chars-backward " \t")
+                   ;; Leave at least `comment-inline-offset' space after
+                   ;; other nonwhite text on the line.
+                   (if (bolp) 0 (+ comment-inline-offset (current-column)))))))
     ;; Fix up the range.
     (if (< max min) (setq max min))
     ;; Don't move past the fill column.
@@ -750,13 +758,6 @@ (defun comment-indent (&optional continue)
 	  ;; If the comment is at the right of code, adjust the indentation.
 	  (unless (save-excursion (skip-chars-backward " \t") (bolp))
 	    (setq indent (comment-choose-indent indent)))
-	  ;; Update INDENT to leave at least one space
-	  ;; after other nonwhite text on the line.
-	  (save-excursion
-	    (skip-chars-backward " \t")
-	    (unless (bolp)
-	      (setq indent (max indent
-                                (+ (current-column) comment-inline-offset)))))
 	  ;; If that's different from comment's current position, change it.
 	  (unless (= (current-column) indent)
 	    (delete-region (point) (progn (skip-chars-backward " \t") (point)))
-- 
2.11.1

Message #49 received at 385 <at> debbugs.gnu.org (full text, mbox):

From: npostavs <at> users.sourceforge.net
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: "Christopher J. Madsen" <cjm <at> cjmweb.net>, 385 <at> debbugs.gnu.org
Subject: Re: bug#385: [PATCH] comment-indent doesn't respect
 comment-indent-function
Date: Wed, 05 Jul 2017 22:58:31 -0400

tags 385 fixed
close 385 26.1
quit

npostavs <at> users.sourceforge.net writes:

> Stefan Monnier <monnier <at> iro.umontreal.ca> writes:
>
>> If you need more control over the placement, rather than a variable
>> comment-indent-fixed, maybe we should just say that if
>> comment-indent-function returns a list of a single integer, it should be
>> taken as the indentation position and not second-guessed.  Or it could
>> return a cons cell (MIN . MAX) to say "anywhere between MIN and MAX".
>
> Here's a patch.

Pushed to master.

[1: e832febfb4]: 2017-07-05 22:52:35 -0400
  Allow comment-indent-functions to specify exact indentation (Bug#385)
  http://git.savannah.gnu.org/cgit/emacs.git/commit/?id=e832febfb4089418e0152c805e24dee977a7590d

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.