Skip to content

update vim indent file #17212

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Sep 22, 2014
Merged

update vim indent file #17212

merged 2 commits into from
Sep 22, 2014

Conversation

mahkoh
Copy link
Contributor

@mahkoh mahkoh commented Sep 12, 2014

There are currently two huge problems with the indent file:

  1. Long list-like things cannot be indented. See Vim indentation broken with long match #14446 for one example. Another one is long enums with over 100 lines, including comments. The indentation process stops after 100 lines and the rest is in column 0.
  2. In certain files, opening a new line at mod level is extremely slow. See this for an example. Opening a line at the very end and holing down will freeze vim temporarily.

The reason for 1. is that cindent doesn't properly indent things that end with a , and the indent file tries to work around this by using the indentation of the previous line. It does this by recursively calling a function on the previous lines until it reaches the start of the block. Naturally O(n^2) function calls don't scale very well. Instead of recalculating the indentation of the previous line, we will now simply use the given indentation of the previous line and let the user deal with the rest. This is sufficient unless the user manually mis-indents a line.

The reason for 2. seems to be function calls of the form

searchpair('{\|(', '', '}\|)', 'nbW', 's:is_string_comment(line("."), col("."))')

I've no idea what this even does or why it is there since I cannot reproduce the mistake cindent is supposed to make without this fix. Therefore I've simply removed that part.

@mahkoh
Copy link
Contributor Author

mahkoh commented Sep 12, 2014

cc @chrismorgan

edit: Wrong Morgan.

@mahkoh
Copy link
Contributor Author

mahkoh commented Sep 12, 2014

cc @chris-morgan

@chris-morgan
Copy link
Member

Interesting. With up-to-date Vim I’m genuinely not getting the behaviour I got a year ago when I wrote it. I suspect that the cinoptions J flag has been fixed since then so that it handles this stuff properly itself. So I approve of the fix for part 2.

As for part 1, you’re right; the recursive O(n²) call was never a good idea.

LGTM.

@lilyball
Copy link
Contributor

This breaks the indentation of the example in the deleted code. Specifically, it no longer indents code inside brackets at the top-level. The actual comment shows how cindent works alone. With your code, nothing is indented, except, curiously, the ]; actually does get indented if the previous line ends in ,:

static FOO: &'static [bool] = [
true,
false,
false,
true,
    ];

The s:is_string_comment stuff is so the searchpair() call doesn't treat delimiters found inside strings and comments as matching pairs, which is necessary in order to accurately determine if it's at the top-level.

@lilyball
Copy link
Contributor

For comparison, cindent:

static FOO : &'static [bool] = [
true,
    false,
    false,
    true,
    ];

Current behavior:

static FOO : &'static [bool] = [
    true,
    false,
    false,
    true,
];

@lilyball
Copy link
Contributor

FWIW, a timeout could be provided to the searchpair() calls to try and fix issue 2. I'm not sure if there's a way to detect that it timed out rather than finding a match.

@chris-morgan
Copy link
Member

@kballard It doesn’t break it for me, on Vim 7.4.410; there, I cannot find anything changed by the removal of what was a workaround. Really, the whole thing was stuff that the J cinoptions value should have been dealing with, but wasn’t—I presume it’s been fixed some time comparatively recently.

@mahkoh
Copy link
Contributor Author

mahkoh commented Sep 16, 2014

Looks like https://code.google.com/p/vim/source/detail?r=9a4efda75b5ef0f496d6a29c0a4dfcc7c03412f9

8825    +           /*
8826    +            * If the previous line ends on '[' we are probably in an
8827    +            * array constant:
8828    +            * something = [
8829    +            *     234,  <- extra indent
8830    +            */
8831    +           if (cin_ends_in(l, (char_u *)"[", NULL))
8832    +           {
8833    +               amount = get_indent() + ind_continuation;
8834    +               break;
8835    +           }

@chris-morgan
Copy link
Member

I’m in favour of removing the workaround as is done here and telling people to update Vim if they complain. The performance problem of working in the module scope is a genuine problem and can only be fixed by removing it.

@mahkoh
Copy link
Contributor Author

mahkoh commented Sep 18, 2014

The vim fix is from July 3rd and there are probably many people (on Debian or Ubuntu) who don't have this version yet. Maybe you can detect the version in the script?

@chris-morgan
Copy link
Member

LGTM.

@lilyball
Copy link
Contributor

I can't test current Vim, because there hasn't been a new MacVim snapshot since patchlevel 258 and I don't really feel like building it myself, but the current behavior seems to be preserved. 👍

bors added a commit that referenced this pull request Sep 22, 2014
There are currently two huge problems with the indent file:

1. Long list-like things cannot be indented. See #14446 for one example. Another one is long enums with over 100 lines, including comments. The indentation process stops after 100 lines and the rest is in column 0.
2. In certain files, opening a new line at mod level is extremely slow. See [this](https://github.com/mahkoh/posix.rs/blob/master/src/unistd/mod.rs) for an example. Opening a line at the very end and holing \<cr> down will freeze vim temporarily.

The reason for 1. is that cindent doesn't properly indent things that end with a `,` and the indent file tries to work around this by using the indentation of the previous line. It does this by recursively calling a function on the previous lines until it reaches the start of the block. Naturally O(n^2) function calls don't scale very well. Instead of recalculating the indentation of the previous line, we will now simply use the given indentation of the previous line and let the user deal with the rest. This is sufficient unless the user manually mis-indents a line.

The reason for 2. seems to be function calls of the form
```
searchpair('{\|(', '', '}\|)', 'nbW', 's:is_string_comment(line("."), col("."))')
```
I've no idea what this even does or why it is there since I cannot reproduce the mistake cindent is supposed to make without this fix. Therefore I've simply removed that part.
@bors bors closed this Sep 22, 2014
@bors bors merged commit 39116d0 into rust-lang:master Sep 22, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants