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

porting 7.4.{2055,2057,2058} #5081

Open
ZyX-I opened this Issue Jul 17, 2016 · 15 comments

Comments

Projects
None yet
4 participants
@ZyX-I
Contributor

ZyX-I commented Jul 17, 2016

If ported, it should be src/nvim/eval/list.c, not src/nvim/list.c, also list definitions are to be moved to eval/list.h. (Same for dictionaries.)

Though I once wanted to move most of the typval_T-related stuff to src/nvim/eval/typval.{c,h}, not split them per-type. Specifically, what Bram moved to list.c and dict.c, clear_tv, GC, probably something else found after inspecting what is left.

@ZyX-I

This comment has been minimized.

Contributor

ZyX-I commented Jul 17, 2016

Actually second paragraph is a question whether 2057 and 2055 are to be ported or my variant should be accepted instead.

@ZyX-I

This comment has been minimized.

Contributor

ZyX-I commented Jul 17, 2016

Also 2058.

@ZyX-I

This comment has been minimized.

Contributor

ZyX-I commented Jul 18, 2016

2063 (that splits internal functions) is not the case, but it should be eval/funcs.{c,h}. Though I found it moves tv_islocked somewhere which should belong to eval/typval.c… Guess no patches should be ported (automatically, I mean), refactoring should be redone instead.

@ZyX-I

This comment has been minimized.

Contributor

ZyX-I commented Jul 24, 2016

I am currently doing refactoring and have a different plan:

  • eval/typval: base typval_T manipulations
  • eval/executor: ex_eval plus functions like ex_let which for some reason live in eval.c.
  • eval/expressions: evalN functions. To be replaced with implementation that is based on similar file from luaviml branch.
  • eval/gc: GC, obviously.

To be completed in a number of PRs.

Ping @justinmk

@justinmk

This comment has been minimized.

Member

justinmk commented Jul 24, 2016

@ZyX-I Completely fine with me.

@siddharth2610

This comment has been minimized.

siddharth2610 commented Sep 3, 2017

Is anyone working on this issue or can i work on this

@justinmk

This comment has been minimized.

Member

justinmk commented Sep 3, 2017

@siddharth2610 Please go ahead. Try to send small, isolated changes, not one giant change. Especially in the beginning, so that we can discuss sooner than later.

@siddharth2610

This comment has been minimized.

siddharth2610 commented Sep 3, 2017

Can you give some information about where to start from

@justinmk

This comment has been minimized.

Member

justinmk commented Sep 3, 2017

If the information here and in CONTRIBUTING.md isn't already enough, I think this is not the task one should start with :)

@jamessan

This comment has been minimized.

Member

jamessan commented Sep 3, 2017

@ZyX-I What parts of this are still relevant after your eval/{executor,typval} refactor?

@ZyX-I

This comment has been minimized.

Contributor

ZyX-I commented Sep 3, 2017

@jamessan Everything: eval/typval was refactored, others net (also eval/executor and eval/gc). Minus eval/expressions: I am currently writing a proper parser now.

@justinmk

This comment has been minimized.

Member

justinmk commented Sep 3, 2017

Based on #5081 (comment) is this still entry-level? And does this issue need to remain open?

Personally if I spent time reviewing entry-level patches for this, I would rather spend the time doing the work myself. Guiding others about what needs to be done here may take more time than it saves.

@ZyX-I

This comment has been minimized.

Contributor

ZyX-I commented Sep 3, 2017

Moving things like f_… elsewhere is easy, but takes time. Should be still entry-level.

@justinmk justinmk removed the question label Sep 3, 2017

@justinmk justinmk modified the milestones: todo, 0.3 Sep 3, 2017

@justinmk justinmk changed the title from Note on porting 7.4.2057 and .2055 to Note on porting 7.4.{2055,2057,2058} Sep 3, 2017

@justinmk justinmk changed the title from Note on porting 7.4.{2055,2057,2058} to porting 7.4.{2055,2057,2058} Sep 3, 2017

@justinmk

This comment has been minimized.

Member

justinmk commented Sep 3, 2017

@siddharth2610 So here is a task to start with: create two new files eval/funcs.c and eval/funcs.h. Using vim/vim@73dad1e#diff-ab0e9d9a1cba9d54d8f6e98913901833 as a reference, move some functions from eval.c to eval/funcs.c. For example, get_function_name() and get_expr_name().

Usually this will also mean fixing the lint errors. Use make lint locally to check lint errors.

@siddharth2610

This comment has been minimized.

siddharth2610 commented Sep 3, 2017

Ok sure .

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