[RFC] Vgetorpeek disect #6370

hardenedapple · 2017-03-26T20:03:59Z

I'm in the middle of trying to understand vgetorpeek(), and in order to do so I'm refactoring it into a form I can read easier.

I thought I'd open a PR to see if people want the refactoring in the main branch and to take advantage of the multiple tests runs.

I can see that refactoring in such a low-level function may feel too dangerous too include.

I haven't done much as yet -- most of splitting into separate functions hasn't been started yet.

justinmk · 2017-03-26T20:14:43Z

👍 Try to keep the PR as small as possible, and avoid style changes where a line would otherwise be untouched. Then we can merge (and review more easily) the incremental steps.

hardenedapple · 2017-03-27T10:36:51Z

Just a note for anyone viewing the commits: adding ?w=1 to the end of a commit URL gives the diff ignoring whitespace.

This is especially useful for those commits adding/removing a block separation.

hardenedapple · 2017-03-28T10:22:25Z

I made a mistake in 90fed0a that I fixed in 18697a9 .

I now have the code mostly readable, so I'm going to slowly rebase to remove that mistake. That's when I'll make sure each commit passes all automated tests.

Just mentioning that here so that anyone having a look at this PR understands where all the commits go after this point.

hardenedapple · 2017-03-28T16:14:05Z

That was an accidental close -- I ran git-push with the base commit in the hopes the travis tests would be run on something without any changes.

hardenedapple · 2017-04-05T09:11:02Z

While I didn't keep the entire PR small, each step is really small, so reviewing and merging individual commits should be easy.

bfredl · 2017-04-05T09:21:19Z

82 commits is a bit much to review commits individually, maybe logically similar commits can be squashed? Also small stylistic/clint changes can be squashed together with a preceding related change, no need for a separate commit just for joining two lines, for instance.

bfredl · 2017-04-05T09:22:59Z

src/nvim/getchar.c

-              if ((*mb_ptr2cells)(ptr + col) > 1)
-                --curwin->w_wcol;
+            vcol += lbr_chartabsize(ptr, ptr + col, (colnr_T)vcol);
+            if (has_mbyte) {


you can use that has_mbyte is always true in nvim.

bfredl · 2017-04-05T09:28:53Z

src/nvim/getchar.c

-                --curwin->w_wcol;
+            vcol += lbr_chartabsize(ptr, ptr + col, (colnr_T)vcol);
+            if (has_mbyte) {
+              col += (*mb_ptr2len)(ptr + col);


Also with larger refactorings we should change (*mb_func)( and MB_FUNC( to to mb_func(. The only reason against would be merging from vim easier but with heavy refactoring applying patches must be done manually anyway.

hardenedapple · 2017-04-06T15:09:44Z

I've merged a bunch of logically similar commits together as suggested.

aktau · 2017-04-07T19:09:16Z

src/nvim/getchar.c

+  return p_tm;
+}
+
+static int handle_int(const int advance)


While I appreciate that int is often understood as interrupt and brevity is important, I think it would make sense to name this one handle_interrupt.

The block only did anything if (mp == NULL), but was entered if (mp == NULL || <something>), so remove the <something>.

After a lot of small changes it becomes apparent that this value a) is not used anywhere else, and b) has no stored state over different loop iterations.

It's a little superfluous, but it means that no-one has to look in check_togglepaste() to see that if a mapping has been found it does nothing. This makes it clearer when viewing look_in_typebuf() that mappings are treated preferentially.

Separate the uses into measuring the number of bytes read by inchar() and returning to vgetorpeek() the next character to use. The only time that it matters what value vgetorpeek() ends up with for 'c' is when it's breaking out of the for loop. This is only when get_key_from_user() is returning 1.

It's never used as a value in the for(;;) loop -- it's just set to the return value before signalling that vgetorpeek() has to break out of the loop and return.

I had added it to demonstrate that showcmd_len, wait_tb_len, and c1, were only ever used in that block. After more refactoring it's obvious because this block is now at the end of a function.

Because this block was the last in the for loop, we don't need to distinguish between `carry on` and `continue;`. Hence I can substitute all `return 0;` statements with `return -1;` and use returning `0` as a valid value to set 'c'.

Initially it was in the middle of things, so it needed to specify continue/break/fall-through etc, but now it's at the end of a function, and given the use of the function, we can remove most flow-control from it. This leaves the if/else clause only updating things as required.

First step to putting this in separate function.

Use local_State in the LANGMAP_ADJUST() condition Separate a long if clause into many Set `mode_deleted` and `timedout` to be bools

Improvements in comments. Rename handle_int() -> handle_interrupt() Show keylen is set to 0 in find_typed_map() when no mapping found. Replace NUL with 0.

As he pointed out, there's no real benefit to having it as a separate function.

Rather than pass values by reference into functions that only need to update those values, we don't give functions that information, and make all updates in the calling function by copying members from a returned structure. This makes it much clearer what information a function may be affected by, and what information is updated. Also use an enum in a few places, to limit the information a function is given.

Easier to work on top of, and if that's not taken on board then the likelyhood of this being taken on board is pretty low.

janlazo · 2020-11-14T22:08:32Z

Vim patches 8.1.1792, 8.1.1797 refactor vgetorpeek. Reopen this if porting those patches is insufficient.

justinmk added this to the 0.2.1 milestone Mar 26, 2017

marvim added the WIP label Mar 26, 2017

hardenedapple force-pushed the vgetorpeek-disect branch 2 times, most recently from 1f8b808 to 9241309 Compare March 28, 2017 14:56

hardenedapple closed this Mar 28, 2017

hardenedapple force-pushed the vgetorpeek-disect branch from 9241309 to f9a31e9 Compare March 28, 2017 16:11

jszakmeister removed the WIP label Mar 28, 2017

hardenedapple reopened this Mar 28, 2017

marvim added the WIP label Mar 28, 2017

hardenedapple force-pushed the vgetorpeek-disect branch 7 times, most recently from 48f4ca2 to 726f9da Compare April 3, 2017 15:57

hardenedapple changed the title ~~[WIP] Vgetorpeek disect~~ [RFC] Vgetorpeek disect Apr 5, 2017

bfredl reviewed Apr 5, 2017

View reviewed changes

marvim added RFC and removed WIP labels Apr 5, 2017

hardenedapple force-pushed the vgetorpeek-disect branch from c797be6 to 3dfa1ae Compare April 6, 2017 14:55

aktau reviewed Apr 7, 2017

View reviewed changes

hardenedapple added 18 commits April 25, 2017 10:24

Remove superfluous nested if statement

32248c1

The block only did anything if (mp == NULL), but was entered if (mp == NULL || <something>), so remove the <something>.

mp_match_len is now only used in find_typed_map()

9fb49d4

After a lot of small changes it becomes apparent that this value a) is not used anywhere else, and b) has no stored state over different loop iterations.

max_mlen is an unused variable -- remove it

9aee194

'c' in vgetorpeek() doesn't need to be reset each loop

f1aae22

It's never used as a value in the for(;;) loop -- it's just set to the return value before signalling that vgetorpeek() has to break out of the loop and return.

Remove a separating block

dd75bd0

I had added it to demonstrate that showcmd_len, wait_tb_len, and c1, were only ever used in that block. After more refactoring it's obvious because this block is now at the end of a function.

insert-mode special case for ESC separate function

c927733

Rename a variable to show what it does

1f03121

Move pretty_partial_match into if block

d08ba13

First step to putting this in separate function.

Move handling showcmd into a separate function

65f4531

Misc reordering

ef36f5e

Use local_State in the LANGMAP_ADJUST() condition Separate a long if clause into many Set `mode_deleted` and `timedout` to be bools

clint

4e5cafa

mbyte stuff recommended by bfredl

b1f9a66

Suggestions from aktau

30e2aee

Improvements in comments. Rename handle_int() -> handle_interrupt() Show keylen is set to 0 in find_typed_map() when no mapping found. Replace NUL with 0.

Inline do_insert_message() (suggested by philix)

b6cc2b1

As he pointed out, there's no real benefit to having it as a separate function.

hardenedapple force-pushed the vgetorpeek-disect branch from e664ee1 to 3cd65d6 Compare April 25, 2017 10:30

hardenedapple force-pushed the vgetorpeek-disect branch from 3cd65d6 to 8cae556 Compare April 25, 2017 11:20

hardenedapple added a commit to hardenedapple/neovim that referenced this pull request Apr 25, 2017

Apply vgetorpeek-disect (PR neovim#6370) as one commit

c0f22d6

Easier to work on top of, and if that's not taken on board then the likelyhood of this being taken on board is pretty low.

justinmk modified the milestones: 0.2.1, 0.2.2 Sep 16, 2017

justinmk modified the milestones: 0.3.1, 0.3.2 Jun 10, 2018

justinmk mentioned this pull request Aug 28, 2019

options: remove 'pastetoggle' #10867

Closed

justinmk removed the RFC label Jan 28, 2020

janlazo closed this Nov 14, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

[RFC] Vgetorpeek disect #6370

[RFC] Vgetorpeek disect #6370

Uh oh!

hardenedapple commented Mar 26, 2017

Uh oh!

justinmk commented Mar 26, 2017

Uh oh!

hardenedapple commented Mar 27, 2017

Uh oh!

hardenedapple commented Mar 28, 2017

Uh oh!

hardenedapple commented Mar 28, 2017

Uh oh!

hardenedapple commented Apr 5, 2017

Uh oh!

bfredl commented Apr 5, 2017

Uh oh!

bfredl Apr 5, 2017

Uh oh!

bfredl Apr 5, 2017

Uh oh!

hardenedapple commented Apr 6, 2017

Uh oh!

aktau Apr 7, 2017

Uh oh!

janlazo commented Nov 14, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

Uh oh!

[RFC] Vgetorpeek disect #6370

[RFC] Vgetorpeek disect #6370

Uh oh!

Conversation

hardenedapple commented Mar 26, 2017

Uh oh!

justinmk commented Mar 26, 2017

Uh oh!

hardenedapple commented Mar 27, 2017

Uh oh!

hardenedapple commented Mar 28, 2017

Uh oh!

hardenedapple commented Mar 28, 2017

Uh oh!

hardenedapple commented Apr 5, 2017

Uh oh!

bfredl commented Apr 5, 2017

Uh oh!

bfredl Apr 5, 2017

Choose a reason for hiding this comment

Uh oh!

bfredl Apr 5, 2017

Choose a reason for hiding this comment

Uh oh!

hardenedapple commented Apr 6, 2017

Uh oh!

aktau Apr 7, 2017

Choose a reason for hiding this comment

Uh oh!

janlazo commented Nov 14, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants