Re: [gail@tnp-online.de: octave-mod.el: wrong indentation for "IF", "FOR", etc.]

Re: [gail@tnp-online.de: octave-mod.el: wrong indentation for "IF", "FOR", etc.]

[John W. Eaton]

[-- Attachment #1: message body text --]
[-- Type: text/plain, Size: 1189 bytes --]

On 10-Oct-2007, Kurt Hornik wrote:

| >>>>> Richard Stallman writes:
| 
| > Can you please send the apropriate fix to emacs-devel@gnu.org,
| > then ack this message?
| 
| I assume this means that in all functions using looking-at() or
| re-search-forward() with possibly case sensitive regexps one needs to
| let() case-fold-search to nil?

Thanks.

How about the attached patch then?

Here's the original message:

------- Start of forwarded message -------
X-Spam-Status: No, score=0.0 required=5.0 tests=UNPARSEABLE_RELAY 
	autolearn=failed version=3.1.0
Date: Mon, 08 Oct 2007 13:17:49 +0200
From: GAIL <gail@tnp-online.de>
MIME-Version: 1.0
To: bug-gnu-emacs@gnu.org
Content-Type: text/plain; charset=ISO-8859-15; format=flowed
Subject: octave-mod.el: wrong indentation for "IF", "FOR", etc.

In octave-mode, depending on toggle-case-fold-search, octave-indent-line produces either this

IF = 1 ;
x = 2 ;

or that:

IF = 1 ;
   x = 2 ;

(via the function re-search-backward). The second behavior is obviously nonsense. Similar for "FOR" etc.

I verified the behavior for emacs-20.7.1/octave-2.1.34 and emacs 22.1.1/octave 2.9.14.


G.
------- End of forwarded message -------


jwe



[-- Attachment #2: diffs --]
[-- Type: text/plain, Size: 4646 bytes --]

lisp/ChangeLog:

2007-10-10  John W. Eaton  <jwe@octave.org>

	* progmodes/octave-mod.el (octave-looking-at-kw,
	octave-re-search-forward-kw, octave-re-search-backward-kw):
	New functions.
	(octave-in-defun-p, calculate-octave-indent,
	octave-blink-matching-block-open, octave-beginning-of-defun,
	octave-auto-fill): Use octave-looking-at-kw instead of looking-at
	to search for regexps that contain case-sensitive keywords.
	(octave-beginning-of-defun): Use octave-re-search-backward-kw
	instead of re-search-backward to search for regexps that contain
	case-sensitive keywords.
	(octave-scan-blocks): Likewise, for octave-re-search-forward-kw.


Index: lisp/progmodes/octave-mod.el
===================================================================
RCS file: /sources/emacs/emacs/lisp/progmodes/octave-mod.el,v
retrieving revision 1.39
diff -u -u -r1.39 octave-mod.el
--- lisp/progmodes/octave-mod.el	6 Oct 2007 01:48:02 -0000	1.39
+++ lisp/progmodes/octave-mod.el	10 Oct 2007 15:34:27 -0000
@@ -581,13 +581,25 @@
 	(error nil))
       (< pos (point)))))
 
+(defun octave-looking-at-kw (regexp)
+  (let ((case-fold-search nil))
+    (looking-at regexp)))
+
+(defun octave-re-search-forward-kw (regexp)
+  (let ((case-fold-search nil))
+    (re-search-forward regexp nil 'move inc)))
+
+(defun octave-re-search-backward-kw (regexp)
+  (let ((case-fold-search nil))
+    (re-search-backward regexp nil 'move inc)))
+
 (defun octave-in-defun-p ()
   "Return t if point is inside an Octave function declaration.
 The function is taken to start at the `f' of `function' and to end after
 the end keyword."
   (let ((pos (point)))
     (save-excursion
-      (or (and (looking-at "\\<function\\>")
+      (or (and (octave-looking-at-kw "\\<function\\>")
 	       (octave-not-in-string-or-comment-p))
 	  (and (octave-beginning-of-defun)
 	       (condition-case nil
@@ -658,14 +670,14 @@
 		(while (< (point) eol)
 		  (if (octave-not-in-string-or-comment-p)
 		      (cond
-		       ((looking-at "\\<switch\\>")
+		       ((octave-looking-at-kw "\\<switch\\>")
 			(setq icol (+ icol (* 2 octave-block-offset))))
-		       ((looking-at octave-block-begin-regexp)
+		       ((octave-looking-at-kw octave-block-begin-regexp)
 			(setq icol (+ icol octave-block-offset)))
-		       ((looking-at octave-block-else-regexp)
+		       ((octave-looking-at-kw octave-block-else-regexp)
 			(if (= bot (point))
 			    (setq icol (+ icol octave-block-offset))))
-		       ((looking-at octave-block-end-regexp)
+		       ((octave-looking-at-kw octave-block-end-regexp)
 			(if (not (= bot (point)))
 			    (setq icol (- icol
 					  (octave-block-end-offset)))))))
@@ -675,10 +687,10 @@
     (save-excursion
       (back-to-indentation)
       (cond
-       ((and (looking-at octave-block-else-regexp)
+       ((and (octave-looking-at-kw octave-block-else-regexp)
 	     (octave-not-in-string-or-comment-p))
 	(setq icol (- icol octave-block-offset)))
-       ((and (looking-at octave-block-end-regexp)
+       ((and (octave-looking-at-kw octave-block-end-regexp)
 	     (octave-not-in-string-or-comment-p))
 	(setq icol (- icol (octave-block-end-offset))))
        ((or (looking-at "\\s<\\s<\\s<\\S<")
@@ -854,8 +866,8 @@
     (save-excursion
       (while (/= count 0)
 	(catch 'foo
-	  (while (or (re-search-forward
-		      octave-block-begin-or-end-regexp nil 'move inc)
+	  (while (or (octave-re-search-forward-kw
+		      octave-block-begin-or-end-regexp)
 		     (if (/= depth 0)
 			 (error "Unbalanced block")))
 	    (if (octave-not-in-string-or-comment-p)
@@ -974,7 +986,7 @@
 	     (looking-at "\\>")
 	     (save-excursion
 	       (skip-syntax-backward "w")
-	       (looking-at octave-block-else-or-end-regexp)))
+	       (octave-looking-at-kw octave-block-else-or-end-regexp)))
 	(save-excursion
 	  (cond
 	   ((match-end 1)
@@ -1021,11 +1033,11 @@
 	 (inc (if (> arg 0) 1 -1))
 	 (found))
     (and (not (eobp))
-	 (not (and (> arg 0) (looking-at "\\<function\\>")))
+	 (not (and (> arg 0) (octave-looking-at-kw "\\<function\\>")))
 	 (skip-syntax-forward "w"))
     (while (and (/= arg 0)
 		(setq found
-		      (re-search-backward "\\<function\\>" nil 'move inc)))
+		      (octave-re-search-backward-kw "\\<function\\>")))
       (if (octave-not-in-string-or-comment-p)
 	  (setq arg (- arg inc))))
     (if found
@@ -1078,7 +1090,7 @@
 	    (save-excursion
 	      (beginning-of-line)
 	      (and auto-fill-inhibit-regexp
-		   (looking-at auto-fill-inhibit-regexp))))
+		   (octave-looking-at-kw auto-fill-inhibit-regexp))))
 	nil				; Can't do anything
       (if (and (not (octave-in-comment-p))
 	       (> (current-column) fc))

[-- Attachment #3: Type: text/plain, Size: 142 bytes --]

_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/emacs-devel

Re: [gail@tnp-online.de: octave-mod.el: wrong indentation for "IF", "FOR", etc.]

[Richard Stallman]

The patch looks good to me.  Would someone please install it?

Re: [gail@tnp-online.de: octave-mod.el: wrong indentation for "IF", "FOR", etc.]

[Glenn Morris]

"John W. Eaton" wrote:

> 2007-10-10  John W. Eaton  <jwe@octave.org>
>
> 	* progmodes/octave-mod.el (octave-looking-at-kw,
> 	octave-re-search-forward-kw, octave-re-search-backward-kw):
> 	New functions.

Why so complicated - why not just let-bind case-fold search in the
functions that need it?

> +(defun octave-re-search-forward-kw (regexp)
> +  (let ((case-fold-search nil))
> +    (re-search-forward regexp nil 'move inc)))
> +
> +(defun octave-re-search-backward-kw (regexp)
> +  (let ((case-fold-search nil))
> +    (re-search-backward regexp nil 'move inc)))

It seems ugly to (ab)use the variable `inc' in this way. If you do
want to do it this way, wouldn't it be nicer to use something like:

(defun octave-re-search-forward (regexp &optional bound noerror count)
  "Like `re-search-forward', but sets `case-fold-search' nil."
  (let (case-fold-search)
     (re-search-forward regexp bound noerror count)))

Re: [gail@tnp-online.de: octave-mod.el: wrong indentation for "IF", "FOR", etc.]

[John W. Eaton]

On 11-Oct-2007, Glenn Morris wrote:

| "John W. Eaton" wrote:
| 
| > 2007-10-10  John W. Eaton  <jwe@octave.org>
| >
| > 	* progmodes/octave-mod.el (octave-looking-at-kw,
| > 	octave-re-search-forward-kw, octave-re-search-backward-kw):
| > 	New functions.
| 
| Why so complicated - why not just let-bind case-fold search in the
| functions that need it?

I wanted to avoid repeating the same code in multiple places.  Isn't
that what subroutines/functions are for?  But in any case, my Emacs
Lisp skills are minimal.  That's one of the reasons I would like for
octave-*.el to live only in the Emacs distribution instead of also in
the Octave sources.  That way, maybe someone who knows Emacs Lisp
better than I can take over maintenance. So please fix it as you see
fit.

| > +(defun octave-re-search-forward-kw (regexp)
| > +  (let ((case-fold-search nil))
| > +    (re-search-forward regexp nil 'move inc)))
| > +
| > +(defun octave-re-search-backward-kw (regexp)
| > +  (let ((case-fold-search nil))
| > +    (re-search-backward regexp nil 'move inc)))
| 
| It seems ugly to (ab)use the variable `inc' in this way. If you do
| want to do it this way, wouldn't it be nicer to use something like:
| 
| (defun octave-re-search-forward (regexp &optional bound noerror count)
|   "Like `re-search-forward', but sets `case-fold-search' nil."
|   (let (case-fold-search)
|      (re-search-forward regexp bound noerror count)))

All the uses of these function are searching for kewords, which is why
I appended the -kw to the name.  I thought it best to simplify the
interface to only what is needed.  I missed inc, so yes, it should
also appear in the argument list.  But since the other two arguments
are always nil and 'move in this context, I don't see why they
need to be exposed in the argument list of the new functions.

jwe

Re: [gail@tnp-online.de: octave-mod.el: wrong indentation for "IF", "FOR", etc.]

[Richard Stallman]

    > +(defun octave-re-search-backward-kw (regexp)
    > +  (let ((case-fold-search nil))
    > +    (re-search-backward regexp nil 'move inc)))

    It seems ugly to (ab)use the variable `inc' in this way.

That is true.  It is not clean to inherit a variable
from a calling function without defvar'ing it, and for that
it should have a longer name.