bug#385: [PATCH] comment-indent doesn't respect comment-indent-function

bug#385: [PATCH] comment-indent doesn't respect comment-indent-function

[Christopher J. Madsen]

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  --------------------

bug#385: [PATCH] comment-indent doesn't respect comment-indent-function

[Stefan Monnier]

> 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

bug#385: [PATCH] comment-indent doesn't respect comment-indent-function

[Christopher J. Madsen]

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  --------------------

bug#385: [PATCH] comment-indent doesn't respect

[Stefan Monnier]

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

bug#385: [PATCH] comment-indent doesn't respect comment-indent-function

[npostavs]

[-- Attachment #1: Type: text/plain, Size: 654 bytes --]

tags 19740 + patch
block 19740 by 385
quit

Stefan Monnier <monnier@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.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: patch --]
[-- Type: text/x-diff, Size: 5164 bytes --]

From cc9db0dbb5590ee909386078128e55c5ee24f319 Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@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.
+
 \f
 * 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

bug#385: [PATCH] comment-indent doesn't respect comment-indent-function

[npostavs]

tags 385 fixed
close 385 26.1
quit

npostavs@users.sourceforge.net writes:

> Stefan Monnier <monnier@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

bug#385: [PATCH] comment-indent doesn't respect comment-indent-function

[Lars Ingebrigtsen]

"Christopher J. Madsen" <cjm@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

bug#385: [PATCH] comment-indent doesn't respect comment-indent-function

[Christopher J. Madsen]

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@cjmweb.net
  --------------------  http://www.cjmweb.net  --------------------