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

[RDY] making the API callable from vimscript #4934

Merged
merged 12 commits into from Sep 1, 2016

Conversation

Projects
None yet
@bfredl
Member

bfredl commented Jun 17, 2016

ref #4091

Instead of generating a f_ wrapper per function, this implements a single api_wrapper function in analogy with float_op_wrapper. Extend the functions table to allow an extra argument to an implementation, this allows several builtin function to share a single implementation (as demonstrated this can be used in many cases for float_op_wrapper as well).

This reuses the handlers implemented for msgpack dispatch. This saves us from generating more code, but means the type checking will be more or less the same. A reason for generating separate f_vim_... wrappers could be to behave more like other builtins with automatic conversion to an int if an int is expected and such (so "somestring" becomes 0 without any error or warning). Though IMO this is not a problem as the names of these functions clearly stand out and hey proper typechecking is nice.

Still the functions need to be inserted in to the table of builtins. We could use #3922 for this to autogenerate this and sort it into the table by lua code.

Arguably buffer nrs should be identified with buffer "handles", otherwise would be a disaster. (It seems reasonable a nvim session would only have <2^31 buffer creation events but I could be wrong...) I would also suggest 0 being a special value always meaning curbuf/curwin etc so one don't need to do buffer_...(bufnr(''), ...) for a one-off api call.

@justinmk

This comment has been minimized.

Member

justinmk commented Jun 17, 2016

A reason for generating separate f_vim_... wrappers could be to behave more like other builtins with automatic conversion to an int if an int is expected and such

I think it's reasonable and even preferable that the API is a bit more rigid.

I would also suggest 0 being a special value always meaning curbuf/curwin

Can't think of a reason against.

I can't shake the idea that it might be useful to somehow namespace the API functions:

  • a global v:api dict (I don't like this idea), e.g. call v:api.vim_get_current_buffer()
  • nvim#vim_get_current_buffer
  • a new "scope style" qualifier, e.g. n:vim_get_current_buffer()
  • put the functions in v:, e.g. v:vim_get_current_buffer()

Maybe it's not useful though, and it's better that they are treated as first-class VimL functions. After all, VimL is itself an implicit namespace.

@justinmk

This comment has been minimized.

Member

justinmk commented Jun 17, 2016

Arguably buffer nrs should be identified with buffer "handles", otherwise would be a disaster. (It seems reasonable a nvim session would only have <2^31 buffer creation events but I could be wrong...)

Is this the reason for replace int64 handle with tp_id, etc.?

@marvim marvim added the WIP label Jun 17, 2016

@bfredl

This comment has been minimized.

Member

bfredl commented Jun 17, 2016

Is this the reason for replace int64 handle with tp_id, etc.?

Not really, I replaced buf->handle with buf->b_fnum and tried to be consistent, but I guess it didn't fall out too well. Another way is to #define b_fnum handle and then when porting 1557 we could just use win->handle instead of w_id internally.

I can't shake the idea that it might be useful to somehow namespace the API functions

I don't have any super strong opinion really, but for me vim_/buffer_ etc is already a "namespace". Remember vim has deprecatedbuffer_number() in favor for bufnr() etc so it seems vim will stay away from these sort of names. Relatedly I thought of blacklisting/whitelisting functions if we don't wan't to expose everyone, to reduce namespace clutter. We could disallow vim_eval() for instance as it is better to be explicit with deepcopy(eval(...)) (is more efficient as well).

@justinmk

This comment has been minimized.

Member

justinmk commented Jun 17, 2016

Remember vim has deprecatedbuffer_number() in favor for bufnr() etc so it seems vim will stay away from these sort of names

You're probably right, I'm overthinking it. The API methods will tend to be very verbose, which is unlikely to conflict with future vim functions. No extra namespace ceremony needed.

Relatedly I thought of blacklisting/whitelisting functions if we don't wan't to expose everyone, to reduce namespace clutter.

👍

@ZyX-I

This comment has been minimized.

Contributor

ZyX-I commented Jun 17, 2016

@bfredl vim_eval is not needed if you want to evaluate expression. But it is a perfect candidate to test how evaluation result would be received on remote end (without actually setting this remote end up). Also deepcopy(eval(…)) and vim_eval(…) have way too much differences in the current state:

  1. No funcrefs support.
  2. No recursive containers support.
  3. If one container is present twice (e.g. let l=[[]]|call add(l, l[0])) after vim_eval there will be two containers.

You're probably right, I'm overthinking it. The API methods will tend to be very verbose, which is unlikely to conflict with future vim functions. No extra namespace ceremony needed.

I would still go for api_*() “namespace”: there are much differences in how values are treated by regular and API functions, above is one of the examples. Such naming is also an explanation to “why there exists both eval() and vim_eval(), what’s the difference?”


I don't have any super strong opinion really, but for me vim_/buffer_ etc is already a "namespace". Remember vim has deprecatedbuffer_number() in favor for bufnr() etc so it seems vim will stay away from these sort of names. Relatedly I thought of blacklisting/whitelisting functions if we don't wan't to expose everyone, to reduce namespace clutter. We could disallow vim_eval() for instance as it is better to be explicit with deepcopy(eval(...)) (is more efficient as well).

Also remember that new Vim functions are named like json_decode() or ch_open(). Or win_getid(). Given that we are likely to merge win_getid() it is also likely that there will be a relevant call in API your variant will be confusing (win_getid() and window_get_id(), which one to use?). Existence of api_window_get_id() here does not look that confusing: one function is Neovim-specific API binding, another one is regular VimL function.

Also note that all variants listed after echo win<Tab> fit in one line and present me unique functions. What will this look like after adding a growing bunch of functions from api/window.c? Same for buf<Tab>.

And you completely forgot about tabpage: new Vim naming scheme is using underscores, but all existing tab* functions start with tabpage*, not tab* like win* or buf* like window- and buffer-related ones. So here naming conflict is a lot more likely.


Regarding function table: I would really suggest to turn this into a hash. I even started doing something like this, but stopped at problem “how to keep khash (or hashtab) reimplementation in lua consistent with it in C code”. Patch is still available: 821958a.

@bfredl

This comment has been minimized.

Member

bfredl commented Jun 17, 2016

But it is a perfect candidate to test how evaluation result would be received on remote end.

Good point.

And you completely forgot about tabpage

Not completely, it is still tabpagenr where the api name would be of the form tabpage_get_number.

For me names like buffer_add_highlight just sound more natural than api_buffer_add_highlight or api_buffer_set_lines. The plugin will add highlights and lines to the buffer, exactly what the name says. Speaking of tabbing, changes are the plugin writer would try buf[fer]<tab> for these operations and not api_buf<tab> . That's also why I mentioned blacklisting, we don't need to import every name into the "global" namespace if we are afraid of cluttering. (to avoid complete duplicates) Also window_get_id() won't exist with this design either as the handle would be the id.

@bfredl

This comment has been minimized.

Member

bfredl commented Jun 17, 2016

Regarding function table: I would really suggest to turn this into a hash. I even started doing something like this, but stopped at problem “how to keep khash (or hashtab) reimplementation in lua consistent with it in C code”.

I though of a hash too, but I thought instead of just initializing the hash with c code (then in principle we could modify msgpack_rpc_add_method_handler to do the part for API functions). I wonder if it really would bring significant time cost to the startup (given that we already build the rpc method table in this way).

@ZyX-I

This comment has been minimized.

Contributor

ZyX-I commented Jun 17, 2016

@bfredl Currently all tabpage* functions have no underscore. But just as well all new functions have underscore like win_getid (older functions look like winheight()), so the point is that name conflict is a lot more likely in the future, not that it is possible right now.

@ZyX-I

This comment has been minimized.

Contributor

ZyX-I commented Jun 17, 2016

@bfredl I do not think this will bring significant cost to the startup, but neither I think that increasing time and memory usage while there is such an easy optimization available is a good idea. Only problem there was “rewriting khash.h in lua” vs “making cross-compiling impossible” vs “adding gperf as a dependency”. Do not remember why I did not post this as a PR with this question: probably 1 or 3 would be accepted.

@bfredl bfredl force-pushed the bfredl:api branch 2 times, most recently from 36aaf6a to f2cb981 Jun 18, 2016

@bfredl

This comment has been minimized.

Member

bfredl commented Jun 18, 2016

@ZyX-I Ok, I went with api_ prefix and I cherry-picked your functions hash branch. I wonder, can't we just require that on cross-compiling the host c compiler is available as well? (Mean, neovim is certainly not the first project that wants to support cross-compiling and also use host c as part of the build process)

Still a lot of expected warnings I hope to go though this weekend (mostly conversion and add dummy void * arguments) but should be working for basic testing.

@ZyX-I

This comment has been minimized.

Contributor

ZyX-I commented Jun 18, 2016

I currently do not know how to do this with cmake. Using gperf seems to be easier, AFAIK it should be available everywhere we currently want to build (linux, windows and mac os).

@bfredl

This comment has been minimized.

Member

bfredl commented Jun 18, 2016

Yes gperf looks what we want to use, but even then we might want to build it as a dependency when building in USE_BUNDLED mode as we do for other dependencies. It seems our third-party cmakelists already defines HOSTDEPS_C_COMPILER for this purpose.

@justinmk

This comment has been minimized.

Member

justinmk commented Jun 18, 2016

Couldn't we commit the result of gperf, instead of requiring gperf as a dependency?

We could automate this in travis:

  • when travis builds any PR, it runs gperf
  • if the result is different than the current source, post it to gist and leave a comment on the PR
    • alternative: leave a comment with instructions for the developer to run gperf locally
  • developer manually commits the result and updates the PR
@bfredl

This comment has been minimized.

Member

bfredl commented Jun 18, 2016

We could, but it seems to just be more complicated for the developer, for no real benifit. We are already requiring (host) lua to be present for building nvim, and building it and running it automatically for the developer, why can't we do the same with gperf? Now, I could imagine shipping the gperf output as part of release tarballs, as the expectaction there is that no modification will be made to the code.

@justinmk

This comment has been minimized.

Member

justinmk commented Jun 18, 2016

We are already requiring (host) lua to be present for building nvim, and building it and running it automatically for the developer, why can't we do the same with gperf?

I don't feel strongly about it, but gperf is a bit more obscure, and our build stack already gets plenty of complaints. Lua doesn't seem comparable: we will eventually ship lua to the end user, so it's not only a build-time dependency.

@bfredl

This comment has been minimized.

Member

bfredl commented Jun 18, 2016

we will eventually ship lua to the end user,

As I suggested neither would end user nor distro people (the ones mostly complaining of our build system, I would presume) need to have gperf if we ship the result as part of the release tarballs, so gperf won't either be a pure bulid-time dependency. So the situation do seem comparable with lua generation to me.

It would only affect the developers situation, and having a small c utility built for you (after a bunch of other c dependencies) seems less burdensome to me than requesting the developer to copy some text from a gist for some reason or getting a weird error message suggesting the developer to build and run gperf themselves.

@bfredl

This comment has been minimized.

Member

bfredl commented Jun 18, 2016

I guess my main point is, the developer does not need to care at all about most of our auto-generating, say, for internal prototypes and the option table, but if the developer happens to change api prototypes now the developer is suddenly forced to care a lot of our process. At least to me this inconsistency would seem a bit odd and hard to explain.

@bfredl bfredl force-pushed the bfredl:api branch 4 times, most recently from 8900835 to ac835b8 Jun 19, 2016

@bfredl

This comment has been minimized.

Member

bfredl commented Jun 26, 2016

I made an attempt of building gperf only if it's needed. The generated hash could be shipped in a release tarball by
cp build/src/nvim/auto/funcs.generated.h src/auto/
which will turn off building gperf. Now the logic is quite ad-hoc (partially as "deps" and "nvim" are two completely separate cmake domains) but this could be extended to cache other generated source or I could imagine enabling shipping a completely static source distribution by cp -r build/src/nvim/auto/ src/auto/ (if we find a reason for it that is)

@bfredl bfredl force-pushed the bfredl:api branch from ac835b8 to be08b0b Jun 28, 2016

@bfredl bfredl changed the title from [WIP] making the API callable from vimscript to [RFC] making the API callable from vimscript Jun 28, 2016

@bfredl

This comment has been minimized.

Member

bfredl commented Jun 28, 2016

The feature itself is more or less "done", so marking RFC.
Some gaps:

  • check if building gperf on mingw works
  • building gpref on msvc: I have no idea what to do here, some windows expert might want to jump in (see third-party/cmake/BuildGperf.cmake)
  • could use a bit more testing
  • make api docs accessible from nvim. doxygen exports machine readable xml, we "just" need to convert it to something reasonably ft=help compatible. (but probably this should be a separate PR)

@bfredl bfredl force-pushed the bfredl:api branch from 533a7fa to acb7c82 Aug 31, 2016

@bfredl

This comment has been minimized.

Member

bfredl commented Aug 31, 2016

Marking as RDY, if no more comments of the code I will merge this, as there are a few PRs wanting to add API functions. (docs I'm not happy with, but as said that will be another PR with auto generating docs for methods, which also will make it easier to reason the surrounding docs)

@bfredl bfredl changed the title from [RFC] making the API callable from vimscript to [RDY] making the API callable from vimscript Aug 31, 2016

@marvim marvim added RDY and removed RFC labels Aug 31, 2016

# define kmalloc(Z) malloc(Z)
# endif
# ifndef krealloc
# define krealloc(P,Z) realloc(P,Z)

This comment has been minimized.

@oni-link

oni-link Sep 1, 2016

Contributor

As a note:
There is a difference in behavior for xrealloc(not NULL ,0) and realloc(not NULL, 0).
realloc() acts here as free(), xrealloc() will always allocate at least one byte.

@bfredl bfredl merged commit c6ac4f8 into neovim:master Sep 1, 2016

4 checks passed

QuickBuild Build pr-4934 finished with status SUCCESSFUL
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.09%) to 70.413%
Details

@jszakmeister jszakmeister removed the RDY label Sep 1, 2016

@justinmk

This comment has been minimized.

Member

justinmk commented Sep 1, 2016

Thanks for this, @bfredl

@piotrpalek

This comment has been minimized.

piotrpalek commented on src/nvim/eval.c in 8fb7273 Sep 2, 2016

I came here from twitter (no idea about neovim codebase) and just noticed that wordcount doesn't have and equivalent entry in the lua file.. just saying in case this is a typo ¯_(ツ)_/¯

This comment has been minimized.

Member

bfredl replied Sep 3, 2016

Sorry about that. It was easier to rebase this thing and keeping track by having the corrections separately, but I guess I should have squashed right before merging...

justinmk added a commit to justinmk/neovim that referenced this pull request Oct 27, 2016

NVIM v0.1.6
FEATURES:
0b5a7e4 neovim#4432 API: external UIs can render custom popupmenu
c6ac4f8 neovim#4934 API: call any API method from vimscript
31df051 neovim#4568 API: nvim_call_atomic(): multiple calls in a single request
b268ba3 neovim#5424 API: nvim_win_get_number(), nvim_tabpage_get_number()
e7e2844 has("nvim-1.2.3") checks for a specific Nvim version
522b885 neovim#5295, neovim#5493 `:CheckHealth` checks tmux, terminfo, performance
719dae2 neovim#5384 events: allow event processing in getchar()
f25797f neovim#5386 API: metadata: Nvim version & API level
22dfe69 neovim#5389 API: metadata: "since", "deprecated_since"
605e743 Added QuickFixLine highlight group

CHANGES:
4af6ec7 neovim#5253 perf: Disable clipboard in do_cmdline()
6e9f329 neovim#5299 perf: Skip foldUpdate() in insert-mode.
9d4fcec neovim#5426 perf: Do not auto-update folds for some foldmethods.
eeec0ca neovim#5419 tui: Default to normal-mode cursor shape.

FIXES:
e838452 neovim#5436 tui: Fix "weird characters" / "bleeding termcodes"
10a54ad neovim#5243 signal_init: Always unblock SIGCHLD.
bccb49b neovim#5316 eval.c: Fix memory leak for detached pty job
626065d neovim#5227 tchdir: New tab should inherit CWD.
cd321b7 neovim#5292 getcwd(): Return empty string if CWD is invalid.
6127eae shada: Fix non-writeable ShaDa directory handling
ca65514 neovim#2789 system(): Respect shellxescape, shellxquote
2daf54e neovim#4874 Restore vim-like tab dragging
0c536b5 neovim#5319 syntax.c: Support bg/fg special color-names.
3c53371 neovim#4972 from justinmk/schedule-ui_refresh
68bcb32 neovim#4789 tui.c: Do not wait for tui loop on teardown.
c8b6ec2 neovim#5409 v:count broken in command-line window
6bc3bce neovim#5461 fix emoji display
51937e1 neovim#5470 fix :terminal with :argadd, :argu
79d77da neovim#5481 external UIs: opening multiple files from command-line
657ba62 neovim#5501 rplugin: resolve paths in manifest file
6a6f188 neovim#5502 system('foo &', 'bar'): Show error, don't crash.
1ff162c neovim#5515 os_nodetype: open fd with O_NONBLOCK
2a6c5bb neovim#5450 modeline: Handle version number overflow.
0ade1bb neovim#5225 CI tests now run against Windows!

justinmk added a commit to justinmk/neovim that referenced this pull request Oct 28, 2016

NVIM v0.1.6
FEATURES:
0b5a7e4 neovim#4432 API: external UIs can render custom popupmenu
c6ac4f8 neovim#4934 API: call any API method from vimscript
31df051 neovim#4568 API: nvim_call_atomic(): multiple calls in a single request
b268ba3 neovim#5424 API: nvim_win_get_number(), nvim_tabpage_get_number()
e7e2844 has("nvim-1.2.3") checks for a specific Nvim version
522b885 neovim#5295, neovim#5493 `:CheckHealth` checks tmux, terminfo, performance
719dae2 neovim#5384 events: allow event processing in getchar()
f25797f neovim#5386 API: metadata: Nvim version & API level
22dfe69 neovim#5389 API: metadata: "since", "deprecated_since"
605e743 Added QuickFixLine highlight group

CHANGES:
4af6ec7 neovim#5253 perf: Disable clipboard in do_cmdline()
6e9f329 neovim#5299 perf: Skip foldUpdate() in insert-mode.
9d4fcec neovim#5426 perf: Do not auto-update folds for some foldmethods.
eeec0ca neovim#5419 tui: Default to normal-mode cursor shape.

FIXES:
e838452 neovim#5436 tui: Fix "weird characters" / "bleeding termcodes"
10a54ad neovim#5243 signal_init: Always unblock SIGCHLD.
bccb49b neovim#5316 eval.c: Fix memory leak for detached pty job
626065d neovim#5227 tchdir: New tab should inherit CWD.
cd321b7 neovim#5292 getcwd(): Return empty string if CWD is invalid.
6127eae shada: Fix non-writeable ShaDa directory handling
ca65514 neovim#2789 system(): Respect shellxescape, shellxquote
2daf54e neovim#4874 Restore vim-like tab dragging
0c536b5 neovim#5319 syntax.c: Support bg/fg special color-names.
3c53371 neovim#4972 from justinmk/schedule-ui_refresh
68bcb32 neovim#4789 tui.c: Do not wait for tui loop on teardown.
c8b6ec2 neovim#5409 v:count broken in command-line window
6bc3bce neovim#5461 fix emoji display
51937e1 neovim#5470 fix :terminal with :argadd, :argu
79d77da neovim#5481 external UIs: opening multiple files from command-line
657ba62 neovim#5501 rplugin: resolve paths in manifest file
6a6f188 neovim#5502 system('foo &', 'bar'): Show error, don't crash.
1ff162c neovim#5515 os_nodetype: open fd with O_NONBLOCK
2a6c5bb neovim#5450 modeline: Handle version number overflow.
0ade1bb neovim#5225 CI tests now run against Windows!

@bfredl bfredl referenced this pull request Apr 30, 2017

Closed

Endianness of EXT types #6620

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment