[RFC] eval: Refactor eval.c #5119

ZyX-I · 2016-07-26T19:59:12Z

ZyX-I · 2016-09-04T00:49:04Z

This part should be mostly ready. Need to

silence linter (376 new errors)
add some unit tests

, but no more function moves in this PR.

bfredl · 2016-09-04T08:24:44Z

src/nvim/CMakeLists.txt

@@ -199,7 +200,7 @@ add_custom_command(OUTPUT ${GENERATED_UNICODE_TABLES}
 )

 add_custom_command(OUTPUT ${GENERATED_API_DISPATCH} ${API_METADATA}
-    COMMAND ${LUA_PRG} ${DISPATCH_GENERATOR} ${PROJECT_SOURCE_DIR}/scripts ${API_HEADERS} ${GENERATED_API_DISPATCH} ${API_METADATA}
+  COMMAND ${LUA_PRG} ${DISPATCH_GENERATOR} ${API_SOURCE_DIR} ${API_HEADERS} ${GENERATED_API_DISPATCH} ${API_METADATA}


Seems unnecessary to have a variable for every subdir, no? if we should follow the existing pattern
set(DEPRECATED_DISPATCH_FILE ${PROJECT_SOURCE_DIR}/src/nvim/api/deprecated_dispatch.lua but when thinking on it why not have set(NVIM_SOURCE_DIR ${PROJECT_SOURCE_DIR}/src/nvim) and then write ${NVIM_SOURCE_DIR}/api/deprecated_dispatch.lua, ${NVIM_SOURCE_DIR}/ex_cmds.lua etc in the build rules?

I am wondering whether there is cmake variable which is like ${PROJECT_SOURCE_DIR}, but contains a directory where currently processed CMakeLists.txt lives. Such thing is better suited here.

From http://www.cmake.org/Wiki/CMake_Useful_Variables:

CMAKE_CURRENT_LIST_DIR
(since 2.8.3) this is the directory of the listfile currently being processed.

ZyX-I · 2016-09-04T16:51:12Z

AppVeyor complains that

C:/projects/neovim/build/include/mbyte.h.generated.h:50:36: error: unknown type name 'WCHAR'
int utf8_to_utf16(const char _str, WCHAR *_strw) FUNC_ATTR_NONNULL_ALL;

, but I did not remove any headers from mbyte.h. Guess that there is some missing include in mbyte.h, where is this type defined on Windows (more precisely, what should I include, I see it defined in winnt.h in headers installed by mingw)?

fwalch · 2016-09-04T18:05:54Z

.ci/common/test.sh

@@ -74,6 +74,10 @@ run_oldtests() {
  valgrind_check "${LOG_DIR}"
 }

+run_single_includes_tests() {
+  ${MAKE_CMD} -C "${BULID_DIR}" check-single-includes


Typo on BUILD_DIR.

This line should probably go into .ci/build.bat, too.

ZyX-I · 2016-09-10T19:41:32Z

It appears that single includes tests used to fail just because I have written singular in place of plural. Wondering why the printed result was that inadequate: out of “.ci/run_tests.sh: line 13: run_single_includes_test: command not found” (with different line though) only “mand not found” was left.

ZyX-I · 2016-09-11T00:42:16Z

Appveyor errors are something inadequate:

In file included from C:/Projects/neovim-1kmqj/src/nvim/mbyte.h:7:0,
                 from C:/Projects/neovim-1kmqj/src/nvim/eval/typval.h:10,
                 from C:\Projects\neovim-1kmqj\src\nvim\eval\decode.c:5:
C:/msys64/mingw64/x86_64-w64-mingw32/include/winnt.h:291:11: error: unknown type name 'CONST'
   typedef CONST WCHAR *LPCWCH,*PCWCH;
           ^

(and ten thousand lines of errors like this): looks like if winnt.h could not be directly included at all.

jamessan · 2016-09-11T01:51:52Z

looks like if winnt.h could not be directly included at all.

In my experience, Windows header's are pretty bad in terms of dependencies on other headers and include order. I'd suggest including Windows.h (which pulls in a lot of other relevant headers) and adding -DWIN32_LEAN_AND_MEAN to the preprocessor defines.

ZyX-I · 2016-09-18T22:46:45Z

Test linter has found an error which for some reason cannot be silenced by -- luacheck: ignore:

local function first_di(d)
  for _, di in dict_iter(d) do
    return di
  end
end

: error is “loop is executed at most once”. I can, of course, remove the syntax sugar behind for … in, but this code is cleaner.

bfredl · 2016-09-25T15:57:35Z

src/nvim/CMakeLists.txt

@@ -204,14 +206,19 @@ add_custom_command(OUTPUT ${GENERATED_UNICODE_TABLES}
 )

 add_custom_command(OUTPUT ${GENERATED_API_DISPATCH} ${API_METADATA}
-    COMMAND ${LUA_PRG} ${DISPATCH_GENERATOR} ${CMAKE_CURRENT_LIST_DIR} ${API_HEADERS} ${GENERATED_API_DISPATCH} ${API_METADATA}
+  COMMAND ${LUA_PRG} ${DISPATCH_GENERATOR} ${CMAKE_CURRENT_LIST_DIR}/api ${API_HEADERS} ${GENERATED_API_DISPATCH} ${API_METADATA}


Isn't it more consistent with just ${CMAKE_CURRENT_LIST_DIR}? just drop the changes to gendispatch.lua and it should work.

Why should it know about which directory file is located in?

it already knows the leaf filename, why not the entire path in the source directory? Someone reading gendispatch.lua (with the other scripts) would like to know where the file is, this is easier if it is consistent with other scripts and use nvimsrcdir, instead of libdir which doesn't say much. Also if some script later wants to use .lua files in several sub-directories the existing pattern is simpler.

bfredl · 2017-03-29T14:37:49Z

why does mark_mb_adjustpos not live in mbyte.c

IMO Ideally mbyte.c should only contain string/char-manipulating functions, not functions dealing with buffers or the screen explicitly (like mb_fix_col which doesn't even use any mbyte functionality, only a purely screen.c convention), so moving adjustpos away is an improvement.

justinmk · 2017-03-29T14:46:09Z

But what does mark_mb_adjustpos have to do with marks?

ZyX-I · 2017-03-29T20:14:03Z

pos_T is a mark position, so it is in mark.c. Any new void* types mean that I failed to find an easy way to fix problems with headers (i.e. header with function definition implicitly depends on another header, but if you just add it there you will find out that you are trying to add a cycle, which type was replaced with void * indicates which header I failed to include). Should be fixed far later, as a part of #6371. Any new moves are out of scope. I would rather omit triggering CI again by removing commits.

ZyX-I · 2017-03-29T20:16:10Z

Actually, mb_check_adjust_col should also be somewhere else. Guess, in cursor.c like function it references on the second line (docstring).

ZyX-I · 2017-03-29T20:17:15Z

And mb_adjust_cursor.

justinmk · 2017-03-29T20:34:46Z

It would be more intuitive if all of the position/cursor adjustment functions go in cursor.c. Regardless of where pos_T happens to come from.

justinmk · 2017-03-29T20:35:25Z

@oni-link any more comments?

ZyX-I · 2017-03-29T21:01:30Z

Would not say so. Cursor may be seen as a mark. But marks can’t be seen as cursors.

oni-link · 2017-03-29T21:06:25Z

@justinmk, not much time at the moment. I'm still stuck in typval.c reviewing, have not seen much else.

justinmk · 2017-03-29T21:14:17Z

@oni-link No problem, didn't want to merge this if you were still looking at it. Let's do it...

jamessan · 2017-03-29T21:17:54Z

src/nvim/move.PVS-Studio.cfg

@@ -0,0 +1,10 @@
+
+source-file=/home/zyx/a.a/Proj/c/neovim/src/nvim/move.c


Should this file be here? :)

Have no idea how it appeared, I definitely did not create it when playing with PVS-Studio.

Especially how it appeared alone. I would understand if there was one cfg per .c file, could not possibly let that sneak in: too easy to see in git st even though I have a number of untracked files. But cfg for only one source file…

oni-link · 2017-04-13T17:09:30Z

src/nvim/eval/typval.c

+/// Get the number value of a VimL object
+///
+/// @note Use tv_get_number_chk() if you need to determine whether there was an
+//


New way to return NULL?

? There are no pointer returns here.

Comment was somehow misplaced. See line 2490 on master:
https://github.com/neovim/neovim/blob/master/src/nvim/eval/typval.c#L2465-L2494

oni-link · 2017-04-16T11:49:20Z

src/nvim/eval/typval.c

+///
+/// @return OK or FAIL.
+int tv_list_concat(list_T *const l1, list_T *const l2, typval_T *const tv)
+  FUNC_ATTR_WARN_UNUSED_RESULT


FUNC_ATTR_NONNULL_ARG(3)

oni-link · 2017-04-16T12:34:27Z

src/nvim/eval/typval.c

+
+typedef struct {
+  char_u *s;
+  char_u *tofree;


Why is tofree needed? Can't we free with s too?

To optimize some strlen() calls out, we could save the length of s here too and later use ga_concat_len() instead of ga_concat() in list_join_inner().

oni-link · 2017-04-16T13:51:37Z

src/nvim/eval/typval.c

+    tv_equal_recurse_limit = 1000;
+  }
+  if (recursive_cnt >= tv_equal_recurse_limit) {
+    tv_equal_recurse_limit--;


The result of this function depends on the first call to it being made with recursive == false to set tv_equal_recurse_limit == 1000, otherwise unequal tvs could compare to true.
Could also happen later on if tv_equal_recurse_limit decreases to 0 and calls to this function are made with recursive == true.

oni-link · 2017-04-16T15:52:11Z

src/nvim/eval/typval.c

+/// @param[out]  ret_error  Location where 1 will be saved if index was not
+///                         found. May be NULL. If everything is OK,
+///                         `*ret_error` is not touched.
+///


Add comment like /// @note Needs to be initialized to false to be useful.?

oni-link · 2017-04-16T15:55:34Z

src/nvim/eval/typval.c

+///                         found. May be NULL. If everything is OK,
+///                         `*ret_error` is not touched.
+///
+/// @return Integer value at the given index or -1.


Could also be 0 if *ret_error == true

FEATURES: bc4a2e1 help, man.vim: "outline" (TOC) feature neovim#5169 58422f1 'guicursor' works in the TUI (and sends info to UIs) neovim#6423 129f107 api: nvim_get_mode() neovim#6247 0b59f98 api/ui: externalize tabline neovim#6583 bc6d868 'listchars': `Whitespace` highlight group neovim#6367 6afa7d6 writefile() obeys 'fsync' option neovim#6427 c60e409 eval.c refactor (also improves some error messages) neovim#5119 9d200cd getcompletion("cmdline") neovim#6376 2ea7bfc terminal: Support extra arguments in 'shell'. neovim#4504 bf51102 DirChanged autocmd neovim#5928 neovim#6262 1743df8 'cpoptions': "_" flag to toggle `cw` behaviour neovim#6235 22337b1 CTRL-R omits trailing ^M when pasting to cmdline neovim#6137 0e44916 :edit allows unescaped spaces in filename neovim#6119 abdbfd2 eval: Add id() function and make printf("%p") useful neovim#6095 bdfa147 findfile(), :find, gf work in :terminal. neovim#6009 2f38ed1 providers: Disable if `g:loaded_*` exists. b5560a6 setpos() can set lowercase marks in other buffers neovim#5753 7c513d6 Throttle :! output, pulse "..." message. neovim#5396 d2e8c76 v:exiting neovim#5651 :terminal improvements neovim#6185 neovim#6142 - cursor keeps position after leaving insert-mode. - 4ceec30 Follows output only if cursor is at end of buffer. - e7bbd35 new option: 'scrollback' - fedb844 quasi-support for undo and 'modifiable' - b45ddf7 disables 'list' by default - disables 'relativenumber' by default :help now contains full API documentation at `:help api`. man.vim saw numerous improvements. Windows support: - Windows is no longer "experimental", it is fully supported. - Windows package includes a GUI, curl.exe and other utilities. "Vim 8" features: partials, lambdas, packages. FIXES: 12fc1de ops: fix i<c-r> with multi-byte text neovim#6524 dd391bf Windows: system() and friends neovim#6497 13352c0 Windows: os_get_hostname() neovim#6413 16babc6 tui: Less-noisy mouse seqs neovim#6411 3a9dd13 (vim bug) folding edge-cases neovim#6207 f6946c6 job-control: set CLOEXEC on pty processes. neovim#5986 d1afd43 rplugin: Call s:LoadRemotePlugins() on startup. 1215084 backtick-expansion works with `shell=fish` neovim#6224 e32ec03 tui: Improved behavior after resize. neovim#6202 86c2adc edit.c: CTRL-SPC: Insert previously-inserted text. neovim#6090 c318d8e b:changedtick now follows VimL rules neovim#6112 34e24cb terminal: Initialize colors in reverse order neovim#6160 e889917 undo: Don't set b_u_curhead in ex_undojoin() neovim#5869 d25649f undo: :earlier, g-: Set b_u_seq_cur correctly. (neovim#6016) 043d8ba 'Visual-mode put from @. register' neovim#5782 42c922b open_buffer(): Do `BufEnter` for directories. 50d0d89 inccommand: Preview :sub commands only after delimiter neovim#5932 1420e10 CheckHealth improvements neovim#5519 c8d5e92 jobstart(): Return -1 if cmd is not executable. neovim#5671 CHANGES: NVIM_TUI_ENABLE_CURSOR_SHAPE was removed. Use 'guicursor' instead. See https://github.com/neovim/neovim/wiki/Following-HEAD#20170402 81525dc 'mouse=a' is no longer the default. (This will probably change again after it is improved.) neovim#6022 0c1f783 defaults: 'showcmd', 'belloff', 'ruler' neovim#6087 eb0e94f api: {get,set}_option update local options as appropriate neovim#6405 bdcb2a3 "Reading from stdin..." message was removed. neovim#6298

FEATURES: bc4a2e1 help, man.vim: "outline" (TOC) feature neovim#5169 58422f1 'guicursor' works in the TUI (and sends info to UIs) neovim#6423 129f107 api: nvim_get_mode() neovim#6247 0b59f98 api/ui: externalize tabline neovim#6583 bc6d868 'listchars': `Whitespace` highlight group neovim#6367 6afa7d6 writefile() obeys 'fsync' option neovim#6427 c60e409 eval.c refactor (also improves some error messages) neovim#5119 9d200cd getcompletion("cmdline") neovim#6376 2ea7bfc terminal: Support extra arguments in 'shell'. neovim#4504 bf51102 DirChanged autocmd neovim#5928 neovim#6262 1743df8 'cpoptions': "_" flag to toggle `cw` behaviour neovim#6235 22337b1 CTRL-R omits trailing ^M when pasting to cmdline neovim#6137 0e44916 :edit allows unescaped spaces in filename neovim#6119 abdbfd2 eval: Add id() function and make printf("%p") useful neovim#6095 bdfa147 findfile(), :find, gf work in :terminal. neovim#6009 2f38ed1 providers: Disable if `g:loaded_*` exists. b5560a6 setpos() can set lowercase marks in other buffers neovim#5753 7c513d6 Throttle :! output, pulse "..." message. neovim#5396 d2e8c76 v:exiting neovim#5651 :terminal improvements neovim#6185 neovim#6142 - cursor keeps position after leaving insert-mode. - 4ceec30 Follows output only if cursor is at end of buffer. - e7bbd35 new option: 'scrollback' - fedb844 quasi-support for undo and 'modifiable' - b45ddf7 disables 'list' by default - disables 'relativenumber' by default :help now contains full API documentation at `:help api`. man.vim saw numerous improvements. Windows support: - Windows is no longer "experimental", it is fully supported. - Windows package includes a GUI, curl.exe and other utilities. "Vim 8" features: partials, lambdas. SECURITY FIXES: CVE-2017-5953 CVE-2017-6349 CVE-2017-6350 neovim#6485 CHANGES: NVIM_TUI_ENABLE_CURSOR_SHAPE was removed. Use 'guicursor' instead. See https://github.com/neovim/neovim/wiki/Following-HEAD#20170402 81525dc 'mouse=a' is no longer the default. (This will probably change again after it is improved.) neovim#6022 0c1f783 defaults: 'showcmd', 'belloff', 'ruler' neovim#6087 eb0e94f api: {get,set}_option update local options as appropriate neovim#6405 bdcb2a3 "Reading from stdin..." message was removed. neovim#6298 FIXES: 12fc1de ops: fix i<c-r> with multi-byte text neovim#6524 dd391bf Windows: system() and friends neovim#6497 13352c0 Windows: os_get_hostname() neovim#6413 16babc6 tui: Less-noisy mouse seqs neovim#6411 3a9dd13 (vim bug) folding edge-cases neovim#6207 f6946c6 job-control: set CLOEXEC on pty processes. neovim#5986 d1afd43 rplugin: Call s:LoadRemotePlugins() on startup. 1215084 backtick-expansion works with `shell=fish` neovim#6224 e32ec03 tui: Improved behavior after resize. neovim#6202 86c2adc edit.c: CTRL-SPC: Insert previously-inserted text. neovim#6090 c318d8e b:changedtick now follows VimL rules neovim#6112 34e24cb terminal: Initialize colors in reverse order neovim#6160 e889917 undo: Don't set b_u_curhead in ex_undojoin() neovim#5869 d25649f undo: :earlier, g-: Set b_u_seq_cur correctly. (neovim#6016) 043d8ba 'Visual-mode put from @. register' neovim#5782 42c922b open_buffer(): Do `BufEnter` for directories. 50d0d89 inccommand: Preview :sub commands only after delimiter neovim#5932 1420e10 CheckHealth improvements neovim#5519 c8d5e92 jobstart(): Return -1 if cmd is not executable. neovim#5671

As part of the refactoring in neovim#5119, some vim_strchr() were changed to strchr(). However, vim_strchr() behaves differently than strchr() when c is NUL, returning NULL instead of a pointer to the NUL. Revert the strchr() calls where it isn't known whether c is NUL, since this causes a semantic change the surrounding code doesn't expect. In the case of neovim#6650, this led to a heap overrun. Closes neovim#6650

As part of the refactoring in #5119, some vim_strchr() were changed to strchr(). However, vim_strchr() behaves differently than strchr() when c is NUL, returning NULL instead of a pointer to the NUL. Revert the strchr() calls where it isn't known whether c is NUL, since this causes a semantic change the surrounding code doesn't expect. In the case of #6650, this led to a heap overrun. Closes #6650

ZyX-I added refactor changes that are not features or bugfixes WIP labels Jul 26, 2016

ZyX-I force-pushed the split-eval branch from b4ebc64 to d640ef4 Compare July 26, 2016 20:02

ZyX-I mentioned this pull request Aug 20, 2016

[RFC] eval: remove char_u in get_dict_(string|number) key parameters #5232

Merged

ZyX-I force-pushed the split-eval branch 2 times, most recently from 401f47a to 13f6129 Compare August 21, 2016 00:07

ZyX-I force-pushed the split-eval branch 4 times, most recently from 0b71f49 to 25f9535 Compare September 4, 2016 00:43

bfredl reviewed Sep 4, 2016
View reviewed changes

fwalch reviewed Sep 4, 2016
View reviewed changes

ZyX-I force-pushed the split-eval branch from 8108b53 to c2ccbca Compare September 5, 2016 06:58

bfredl mentioned this pull request Sep 14, 2016

[RFC] make del_var and set_var not return the old value. #5336

Merged

ZyX-I force-pushed the split-eval branch 2 times, most recently from 0119ca3 to a3205b1 Compare September 17, 2016 01:37

ZyX-I mentioned this pull request Sep 17, 2016

Fix edge cases for NULL typval_T Lists #4615

Closed

30 tasks

ZyX-I force-pushed the split-eval branch from 8309142 to 1beda85 Compare September 18, 2016 22:03

ZyX-I force-pushed the split-eval branch from 4942e70 to 5935a7c Compare September 23, 2016 21:55

bfredl reviewed Sep 25, 2016

View reviewed changes

ZyX-I mentioned this pull request Sep 25, 2016

Support running each test file in a separate processes lunarmodules/busted#278

Closed

justinmk merged commit c60e409 into neovim:master Mar 29, 2017

justinmk removed the RFC label Mar 29, 2017

jamessan reviewed Mar 29, 2017

View reviewed changes

ZyX-I deleted the split-eval branch March 29, 2017 22:02

ZyX-I mentioned this pull request Apr 3, 2017

Change workflow from rebase-based to no-rebase merge-based #6435

Closed

justinmk mentioned this pull request Apr 10, 2017

nvim slow after build 12233 #6437

Closed

oni-link reviewed Apr 13, 2017

View reviewed changes

justinmk mentioned this pull request Apr 15, 2017

remove() with {end} does not return removed items #6521

Closed

oni-link reviewed Apr 16, 2017

View reviewed changes

justinmk mentioned this pull request May 1, 2017

NVIM v0.2.0 #6634

Merged

jamessan mentioned this pull request May 3, 2017

Use vim_strchr(s, c) when c may be NUL #6656

Merged

		@@ -0,0 +1,10 @@

		source-file=/home/zyx/a.a/Proj/c/neovim/src/nvim/move.c

Uh oh!

[RFC] eval: Refactor eval.c #5119

[RFC] eval: Refactor eval.c #5119

Uh oh!

Conversation

ZyX-I commented Jul 26, 2016

Uh oh!

ZyX-I commented Sep 4, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ZyX-I commented Sep 4, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ZyX-I commented Sep 10, 2016

Uh oh!

ZyX-I commented Sep 11, 2016

Uh oh!

jamessan commented Sep 11, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ZyX-I commented Sep 18, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bfredl commented Mar 29, 2017

Uh oh!

justinmk commented Mar 29, 2017

Uh oh!

ZyX-I commented Mar 29, 2017

Uh oh!

ZyX-I commented Mar 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ZyX-I commented Mar 29, 2017

Uh oh!

justinmk commented Mar 29, 2017

Uh oh!

justinmk commented Mar 29, 2017

Uh oh!

ZyX-I commented Mar 29, 2017

Uh oh!

oni-link commented Mar 29, 2017

Uh oh!

justinmk commented Mar 29, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ZyX-I Mar 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jamessan commented Sep 11, 2016 •

edited

Loading

ZyX-I commented Mar 29, 2017 •

edited

Loading

ZyX-I Mar 29, 2017 •

edited

Loading