[vim/vim] The nv_g_cmd() function is too long. Refactor it using a table of key… (PR #9561)
Yegappan Lakshmanan
…s and corresponding handler functions
You can view, comment on, or merge this pull request online at:
https://github.com/vim/vim/pull/9561
Commit Summary
- 2114111 The nv_g_cmd() function is too long. Refactor it using a table of keys and corresponding handler functions
File Changes
(1 file)
- M src/normal.c (1253)
Patch Links:
—
Reply to this email directly, view it on GitHub.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.
codecov[bot]
Codecov Report
Merging #9561 (2114111) into master (17dd519) will decrease coverage by
0.73%.
The diff coverage is93.10%.
@@ Coverage Diff @@ ## master #9561 +/- ## ========================================== - Coverage 83.51% 82.77% -0.74% ========================================== Files 154 153 -1 Lines 174341 173199 -1142 Branches 39220 39227 +7 ========================================== - Hits 145594 143374 -2220 - Misses 16661 17565 +904 - Partials 12086 12260 +174
| Flag | Coverage Δ | |
|---|---|---|
| huge-clang-none | 81.87% <92.69%> (+<0.01%) |
⬆️ |
| huge-gcc-none | ? |
|
| huge-gcc-testgui | 80.71% <89.61%> (-0.01%) |
⬇️ |
| huge-gcc-unittests | 2.03% <0.00%> (+<0.01%) |
⬆️ |
Flags with carried forward coverage won't be shown. Click here to find out more.
| Impacted Files | Coverage Δ | |
|---|---|---|
| src/normal.c | 90.56% <93.10%> (-0.12%) |
⬇️ |
| src/libvterm/src/rect.h | 0.00% <0.00%> (-96.78%) |
⬇️ |
| src/libvterm/src/state.c | 38.86% <0.00%> (-50.30%) |
⬇️ |
| src/libvterm/include/vterm.h | 0.00% <0.00%> (-44.45%) |
⬇️ |
| src/libvterm/src/keyboard.c | 44.21% <0.00%> (-43.42%) |
⬇️ |
| src/libvterm/src/parser.c | 60.83% <0.00%> (-35.06%) |
⬇️ |
| src/libvterm/src/pen.c | 48.81% <0.00%> (-34.90%) |
⬇️ |
| src/libvterm/src/encoding.c | 39.39% <0.00%> (-34.14%) |
⬇️ |
| src/libvterm/src/vterm.c | 39.17% <0.00%> (-27.21%) |
⬇️ |
| src/libvterm/src/utf8.h | 68.18% <0.00%> (-4.55%) |
⬇️ |
| ... and 26 more |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing data
Powered by Codecov. Last update 17dd519...2114111. Read the comment docs.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Bram Moolenaar
I'm not sure this is an improvement. Using a switch statement the compiler can find the optimal way to select the code to execute. A linear search through an array is likely to always be slower.
A binary search would be much better, but it is hard to get the key codes in sorted order.
A kind of hashtable might also work. It would have to be created at runtime though.
We have a similar problem for the normal mode command, which is using a sorted index table. It is filled in init_normal_cmds().
This adds to the startup time.
We could change this to generate the table at compile time. Then the table used for "g" commands could also use a binary search, like in find_command(). The "g" command table would have gaps in the lower block (the part where the typed character can directly be used as an index, up to "~"). Or not use a lower block.
—
Reply to this email directly, view it on GitHub.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.
Yegappan Lakshmanan
@yegappan pushed 4 commits.
- b4868ed patch 8.2.4142: build failure with normal features without persistent undo
- b0b2b73 patch 8.2.4143: MS-Windows: IME support for Win9x is obsolete
- 1a8825d patch 8.2.4144: cannot load libsodium dynamically
- 3b722aa Merge branch 'master' of github.com:yegappan/vim
You are receiving this because you are subscribed to this thread.
Yegappan Lakshmanan
I'm not sure this is an improvement. Using a switch statement the compiler can find the optimal way to select the code to execute. A linear search through an array is likely to always be slower.
A binary search would be much better, but it is hard to get the key codes in sorted order.
A kind of hashtable might also work. It would have to be created at runtime though.We have a similar problem for the normal mode command, which is using a sorted index table. It is filled in init_normal_cmds().
This adds to the startup time.We could change this to generate the table at compile time. Then the table used for "g" commands could also use a binary search, like in find_command(). The "g" command table would have gaps in the lower block (the part where the typed character can directly be used as an index, up to "~"). Or not use a lower block.
vim-dev ML
On Wed, Jan 19, 2022 at 4:34 AM Bram Moolenaar ***@***.***>
—
Reply to this email directly, view it on GitHub.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.
Yegappan Lakshmanan
Closed #9561.
—
Reply to this email directly, view it on GitHub.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you commented.
Bram Moolenaar
Yegappan wrote:
> On Wed, Jan 19, 2022 at 4:34 AM Bram Moolenaar ***@***.***>
> wrote:
>
> > I'm not sure this is an improvement. Using a switch statement the compiler
> > can find the optimal way to select the code to execute. A linear search
> > through an array is likely to always be slower.
> > A binary search would be much better, but it is hard to get the key codes
> > in sorted order.
> > A kind of hashtable might also work. It would have to be created at
> > runtime though.
> >
> > We have a similar problem for the normal mode command, which is using a
> > sorted index table. It is filled in init_normal_cmds().
> > This adds to the startup time.
> >
> > We could change this to generate the table at compile time. Then the table
> > used for "g" commands could also use a binary search, like in
> > find_command(). The "g" command table would have gaps in the lower block
> > (the part where the typed character can directly be used as an index, up to
> > "~"). Or not use a lower block.
> >
> >
> >
> I will rework the patch to use a binary search to lookup the function for a
> given key.
mechanism, but the way it is used can function as an example. The
result is efficient at runtime.
Perhaps we just need a script that can sort the array, so that a binary
search is possible. Or generate the lookup table, so that
init_normal_cmds() isn't needed.
--
LAUNCELOT: I am, sir. I am a Knight of King Arthur.
FATHER: 'Mm ... very nice castle, Camelot ... very good pig country....
"Monty Python and the Holy Grail" PYTHON (MONTY) PICTURES LTD
/// Bram Moolenaar -- Br...@Moolenaar.net -- http://www.Moolenaar.net \\\
/// \\\
\\\ sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ ///
\\\ help me help AIDS victims -- http://ICCF-Holland.org ///
Yegappan Lakshmanan
all strings. But the nv_cmds[] array contains key codes (like Ctrl_A,
K_MOUSEUP, etc). So from an external script, it will be difficult to translate
these codes (which are C macros) to corresponding numbers which can
be sorted.
We can make sure the g command table is sorted (at least upto the '~'
character),
so that the printable character can be used directly as the index and and do a
binary search for the special keycodes.
Regards,
Yegappan
Bram Moolenaar
switch statement, leaving the optimization to the compiler, is the best.
For the first command character we can use the index directly for many
keys, so that makes it fast. It also has many more entries.
For the nv_g_cmd() function I think we can leave it a switch. We can
move the larger code blocks to a function to keep overview. Especially
"$", that is 64 lines. I would think if the code goes over ten lines
it's worth moving to a function.
--
There can't be a crisis today, my schedule is already full.