code review 5844063: misc/emacs: fix overindentation caused by mis-parsing l...

misc/emacs: fix overindentation caused by mis-parsing lines ending with special chars

Fixes issue 3313

go-mode-backward-skip-comments is probably due for a more ambitious refactoring --- it repeats guard conditions after every nearly every movement of point.

[bf, 2012-03-20 02:46:33]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://go.googlecode.com/hg/

[bf, 2012-03-20 14:27:22]

Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),

Please take another look.

[adg, 2012-03-20 23:43:13]

Anyone know elisp? I don't.

[r2, 2012-03-20 23:47:05]

On Mar 21, 2012, at 10:42 AM, Andrew Gerrand wrote:

> Anyone know elisp? I don't.

i hear ben fried does.

-rob

[volker.dobler, 2012-03-21 00:32:59]

At least lines ending in " are still broken:

import (
	"fmt"
"math"
)

and


// foo "bar"
	// next line

[bf, 2012-03-21 01:31:13]

I can tell you in more detail what I did if you wish.

Issue 3313 was fixed by turning on case sensitivity when parsing code to
set indent level --- the regexps that find keywords/reserved words were
being invoked with whatever the user's setting for case-sensitivity in
search is; most people leave it at the default, which is case insensitivity
for regexp matching and interactive searching. I used a let to re-bind
case-fold-search in the outermost indentation function.

The code that calculates indent level is functional, and I think it's now
correct, but it's not terribly pretty. Even if you don't know elisp, a
quick glance shows that in the section I patched it tests for guard
conditions in a suspiciously high number of places, not just at loop top or
bottom. I think it can be rewritten to be simpler and clearer, but
meanwhile I was able to correct an error causing it to misindent lines
following lines that end with a delimited strong. The code works backward
from the point of user input to parse enough of the buffer to calculate the
current indent level, but it attempts to both move backward a character at
a time and also leap across contiguous blocks of whitespace  --- this is
what (skip-syntax-backward...) does --- but that was actually causing
incorrect indentation: the insertion point was being taken past the
terminating string delimiter.

It's a bit late; I hope that is coherent.

Ben
On Mar 20, 2012 7:47 PM, "Rob 'Commander' Pike" <r@google.com> wrote:

>
> On Mar 21, 2012, at 10:42 AM, Andrew Gerrand wrote:
>
> > Anyone know elisp? I don't.
>
> i hear ben fried does.
>
> -rob
>
>

[Sameer Ajmani, 2012-03-22 15:04:30]

http://codereview.appspot.com/5844063/diff/5001/misc/emacs/go-mode.el
File misc/emacs/go-mode.el (right):

http://codereview.appspot.com/5844063/diff/5001/misc/emacs/go-mode.el#newcode410
misc/emacs/go-mode.el:410: (or (eq 32 (char-syntax char))))
Add a function comment.

I think you can remove the (or), since there's only a single argument to it. 
That is, this becomes:
(defun go-mode-whitespace-p (char)
  "Returns true if char's syntax class is whitespace."
  (eq 32 (char-syntax char)))

http://codereview.appspot.com/5844063/diff/5001/misc/emacs/go-mode.el#newcode417
misc/emacs/go-mode.el:417: (or (go-mode-whitespace-p (char-after (point)))
One way to clean up this loop would be to capture the repeated subexpressions in
named variables.

I'm not sure whether this loop is correct, but your cleanup helps make it more
readable.  A few minor suggestions.

http://codereview.appspot.com/5844063/diff/5001/misc/emacs/go-mode.el#newcode419
misc/emacs/go-mode.el:419: (when (and (not (bobp)) (go-mode-whitespace-p
(char-after (point))))
put (go-mode-whitespace-p (char-after (point))) on the next line, indented at
align with (not (bobp)), to make it easier to compare visually against the loop
bound.

http://codereview.appspot.com/5844063/diff/5001/misc/emacs/go-mode.el#newcode424
misc/emacs/go-mode.el:424: (when (and (not (go-mode-cs)) (go-mode-whitespace-p
(char-after (1+ (point)))))
Move (go-mode-whitespace-p ...) to the next line, aligned with (not
(go-mode-cs)), to make it easier to see what's covered by the (and).

[bf, 2012-03-26 02:55:01]

Hello golang-dev@googlegroups.com, adg@golang.org, r@google.com,
dr.volker.dobler@gmail.com, sameer@golang.org (cc: golang-dev@googlegroups.com),

Please take another look.

[bf, 2012-03-26 02:58:40]

Hello golang-dev@googlegroups.com, adg@golang.org, r@google.com,
dr.volker.dobler@gmail.com, sameer@golang.org (cc: golang-dev@googlegroups.com),

Please take another look.

[bf, 2012-03-26 03:07:54]

Hello golang-dev@googlegroups.com, adg@golang.org, r@google.com,
dr.volker.dobler@gmail.com, sameer@golang.org (cc: golang-dev@googlegroups.com),

Please take another look.

[volker.dobler, 2012-03-26 09:40:28]

Much better. Only keys in struct literals are not indented
enough (looks like cases in a switch).

import (
	"fmt"
	"net/url"
)

// foo is "simple"
// indented.
// okay
func foo() bool {
	b := url.URL{
	Scheme: "http",
	Host:   "localhost",
	}
}

On 3/26/12, ben.fried@gmail.com <ben.fried@gmail.com> wrote:
> Hello golang-dev@googlegroups.com, adg@golang.org, r@google.com,
> dr.volker.dobler@gmail.com, sameer@golang.org (cc:
> golang-dev@googlegroups.com),
>
> Please take another look.
>
>
> http://codereview.appspot.com/5844063/
>


-- 
Dr. Volker Dobler

[rsc, 2012-03-26 12:24:23]

R=sameer

Thanks very much, Ben.

[bf, 2012-03-26 12:56:15]

Volker has pointed out that it muffs up indentation of keys in struct
literals. I think I will have a fix for that this afternoon (EDT). I am
close but have to run off to close on my new apartment now.

Ben
On Mar 26, 2012 8:24 AM, <rsc@golang.org> wrote:

> R=sameer
>
> Thanks very much, Ben.
>
>
>
http://codereview.appspot.com/**5844063/<http://codereview.appspot.com/5844063/>
>

[bf, 2012-03-26 21:34:31]

Hello sameer@golang.org (cc: golang-dev@googlegroups.com),

Please take another look.

[Sameer Ajmani, 2012-03-26 23:32:10]

ooo today, will look tomorrow.
On Mar 26, 2012 5:34 PM, <ben.fried@gmail.com> wrote:

> Hello sameer@golang.org (cc: golang-dev@googlegroups.com),
>
> Please take another look.
>
>
>
http://codereview.appspot.com/**5844063/<http://codereview.appspot.com/5844063/>
>

[Sameer Ajmani, 2012-03-27 02:32:44]

Most of my comments are about how to structure the repeated code elements. 
Since this bug is somewhat painful, I'm ok with getting the fix in first and
cleaning up the code in a followup CL.

http://codereview.appspot.com/5844063/diff/15002/misc/emacs/go-mode.el
File misc/emacs/go-mode.el (right):

http://codereview.appspot.com/5844063/diff/15002/misc/emacs/go-mode.el#newcod...
misc/emacs/go-mode.el:205: (when (<= b go-mode-mark-comment-end)
This block of 8 lines appears three times, varying only in go-mode-mark-foo-end
and go-mode-foo.  Worth extracting a function with a docstring explaining what
this block is doing?  Or at minimum three similarly-named functions that are
defined near each other:
  (defun go-mode-mark-clear-cs-cache ...
  (defun go-mode-mark-clear-comment-cache ...
  (defun go-mode-mark-clear-string-cache ...
But a single function that takes go-mode-mark-foo-end and go-mode-foo as args
would be better.

http://codereview.appspot.com/5844063/diff/15002/misc/emacs/go-mode.el#newcod...
misc/emacs/go-mode.el:308: "Return the comment/string state at point POS.  If
point is
This docstring is thr same as that for go-mode-cs.  How does this function
differ?

http://codereview.appspot.com/5844063/diff/15002/misc/emacs/go-mode.el#newcod...
misc/emacs/go-mode.el:375: (let ((last-cs
last-string?

http://codereview.appspot.com/5844063/diff/15002/misc/emacs/go-mode.el#newcod...
misc/emacs/go-mode.el:384: (let ((cs-end			; end of the text property
string-end?

http://codereview.appspot.com/5844063/diff/15002/misc/emacs/go-mode.el#newcod...
misc/emacs/go-mode.el:414: (setq go-mode-mark-string-end pos)))))
All this copy-pasting seems a bit painful to me, as it leaves us open to
bugs/changes being added to one of these functions and not the others.  Where
possible, I'd like to extract functions for common behaviors.

It seems like the main parameters here are the go-mode-mark-foo-end and
go-mode-foo symbols, the respective definitions of foo-end, , and the parameters
to search-forward.

This is ok to leave as-is if consolidation is too messy, but at least make the
names consistent.

http://codereview.appspot.com/5844063/diff/15002/misc/emacs/go-mode.el#newcod...
misc/emacs/go-mode.el:558: loop-guard
You're using tabs for indentation here, but the rest of the file uses two
spaces.  Please change this to stay consistent with the existing code.

[r2, 2012-03-27 02:34:27]

can we please get to a working state fast so we can check this in and clean up
later?

-rob

[bf, 2012-03-27 02:51:21]

I believe it's in a working state. Volker reported a problem with my prior
patch which I've fixed. Waiting for review. It indents correctly, and is
thus a lot more usable more than it  was before

Ben

On Mon, Mar 26, 2012 at 10:34 PM, Rob 'Commander' Pike <r@google.com> wrote:

> can we please get to a working state fast so we can check this in and
> clean up later?
>
> -rob
>
>

[Sameer Ajmani, 2012-03-27 03:17:11]

LGTM

fine to submit this now and clean up in a followup
On Mar 26, 2012 10:34 PM, "Rob 'Commander' Pike" <r@google.com> wrote:

> can we please get to a working state fast so we can check this in and
> clean up later?
>
> -rob
>
>

[rsc, 2012-03-27 03:26:43]

*** Submitted as 0d0d70895037 ***

misc/emacs: fix overindentation caused by mis-parsing lines ending with special
chars

Fixes issue 3313

go-mode-backward-skip-comments is probably due for a more ambitious refactoring
--- it repeats guard conditions after every nearly every movement of point.

R=sameer, r
CC=golang-dev
http://codereview.appspot.com/5844063

Committer: Russ Cox <rsc@golang.org>