[patch] Add entry to 'cinoptions' to control placement of jump labels

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

[patch] Add entry to 'cinoptions' to control placement of jump labels

Manuel König
Hi all,

this is my first post here and my first patch for vim (and any other
open source project in general), and I'm new to these diff/patch tools,
so please tell me if there's something wrong or room for improvement. I
created the patch by doing

$cd vim7
$diff -c src/misc1.c my/vim7/src/misc1.c > misc.diff
$diff -c runtime/doc/indent.txt my/vim7/runtime/doc/indent.txt >
indent.diff
$cat indent.diff misc.diff > cino_jump_labels.diff

and successfully applied it by doing

$cd vim7
$patch -c -p0 <cino_jump_labels.diff

Copied from the description I added to cinoption-values:

JN    Controls placement of jump labels. If N equals -1, the label will
      be placed at column 1. If N is non-negative, the indent of the
      label will be that of a normal statement minus N. All other values
      for N are invalid and the default value will be used instead.
      (default -1).

        cino=               cino=L2             cino=Ls >
          func()              func()              func()
          {                   {                   {
              {                   {                   {
                  stmt;               stmt;               stmt;
          LABEL:                    LABEL:            LABEL:
              }                   }                   }
          }                   }                   }

I declared values of N < -1 as invalid because that allows adding new
options in the future, like N=-2 to lign up jump labels with the
current innermost function (some languages allow nested functions,
therefore "innermost").

Advantages:
* Use cino+=J0 to not break up with foldmethod=indent
* Still possible to visually distinguish labels by using
  something like cino+=Js or cino+=J0.5s. This also doesn't
  break up with foldmethod=indent so severely as if the label
  were at column 1.
* When working with highly indendet code, it can be hard to
  track for the eye to which line of code a label belongs to
  if it's located at column 1

Disadvantages:
* Adds one more entry to cinoptions the user may have to read
  before he finds what he wants to have. But still better than
  reading through all options just to find out in the second last
  point that placement of jump labels is not supported, as it was
  the case for me ;)

I still have some questions that need to be resolved before folding
this patch into vim:

(Q1) All code examples in :help cinoptions-values have a mixed use of
normal spaces and tabs, so that the examples break up if I set tabstop
to something different than 8. In my code example I used tabs to indent
the beginning of the code, and then only used spaces. Is that a
documentation bug or is it expected that tabstop==8 for helpfiles?

(Q2) The text in cinoptions-values uses both column 0 and column 1 for
the "first" column. It should be agreed up on a consistent naming, I
prefer column 1 because there really is no such thing as column 0, only
a char array that's indexed starting at 0.

(Q3) In the file src/misc1.c I'm not sure if I placed the code that
actually performs the label shifting at the right place. You can find
that code by doing 'grep QUESTION' on the patched misc1.c. I admit that
I haven't read all the code above that's responsible for indenting
and would appreciate any help of someone who is knowledgeable with that
portion of code.

I'm not sure if the following is expected to be correct behaviour:

:set tabstop=8
:set cindent
:set cino=J4

/*
 * The next label is still indented at column 8, but it
 * should be at column 4. Admittedly such code doesn't
 * make much sense, but it still smells like a bug.
 *
 * But that also doesn't work in the current version of
 * vim if I don't apply my patch (ie. this label won't be
 * put to column 1), so it's probably OK. The reason is
 * that such labels won't even be recognized as labels
 * by cin_islabel(ind_maxcomment).
 */
if (blah)
        LABEL:
if (asdf)
        stmt;

Thank's for your time :)

--
You received this message from the "vim_dev" maillist.
For more information, visit http://www.vim.org/maillist.php

cino_jump_labels.diff (4K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [patch] Add entry to 'cinoptions' to control placement of jump labels

Manuel König
Sorry, there was a bug in my previous patch that shouldn't have slipped
my eyes, I forgot to add 'break;' in a switch structure... It's fixed
now.

--
You received this message from the "vim_dev" maillist.
For more information, visit http://www.vim.org/maillist.php

cino_jump_labels.diff (4K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [patch] Add entry to 'cinoptions' to control placement of jump labels

Lech Lorens
On 19-Feb-2010 Manuel König <[hidden email]> wrote:
> Sorry, there was a bug in my previous patch that shouldn't have slipped
> my eyes, I forgot to add 'break;' in a switch structure... It's fixed
> now.

I have tested the patch: it works. The source code seems fine. There was
a slight mistake in the documentation (the option is referred to as L in
the example). I'm attaching a patch with the original source code and
updated documentation.

--
Cheers,
Lech

--
You received this message from the "vim_dev" maillist.
For more information, visit http://www.vim.org/maillist.php

cino_jump_labels.diff (3K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [patch] Add entry to 'cinoptions' to control placement of jump labels

Manuel König
Lech Lorens wrote:

> I have tested the patch: it works. The source code seems fine. There
> was a slight mistake in the documentation (the option is referred to
> as L in the example). I'm attaching a patch with the original source
> code and updated documentation.
>

Thanks! I wasn't sure if somebody would look at it at all because there
was no feed back for a week, so thanks for reviewing it.

I first wanted to call the option 'L' for label, but I thought 'J' for
jump label (can't be confused with case label) would be more
descriptive, so that's where the L mistake comes from.

regards,

Manuel

--
You received this message from the "vim_dev" maillist.
For more information, visit http://www.vim.org/maillist.php