[RFC] add kbtree_t and use it for bufhl #5266

bfredl · 2016-08-28T18:07:44Z

As discussed in #5031 this is a separate for PR to cleanup and integrate kbtree_t. Also use it for bufhl to get some coverage. Fixed compiler warnings (though the const warnings might have a better solution than remove const everywhere, I removed them for now as make output is impossible to parse otherwise), though remaining style issues.

@timeyyy for extmark there is two things to note:

The "eliminate unneccesary heap allocation" commit does that. kbtree_t is a zero/calloc-initializable struct just like kvec_t. The changes for extmark is hopefully analogous to the bufhl changes in the same commit. (include the struct directly in buf_T, no explicit initialization needed, kbtree functions need an extra &)

bufhl_mark_adjust shows a way to handle :move (only operation where marks can switch order, currently)

timeyyy · 2016-10-23T20:45:16Z

I want to start using the work here, how should i go about incorporating it into my pull request? should i cherry pick the commits ? That would produce the most straight forward commit history. Otherwise if this gets merged into master i can rebase?

bfredl · 2016-10-24T15:07:12Z

You can cherry-pick commits. Later when this is merged, and you rebase this, git will silently drop those commits and if it doesn't, you should be able to silently ignore the conflicts (as long as you don't add separate new changes to kb_tree.h)

I hope to look into this again soon after the 0.1.6 release, first I need to focus on some api versioning work needed for/after 0.1.6 release.

Hinidu · 2016-11-29T22:21:48Z

src/nvim/lib/kbtree.h

@@ -0,0 +1,396 @@
+#ifndef __AC_KBTREE_H


I'm not the lawyer but it looks illegal to use this source code without the original copyright. For example khash.h, klist.h and kvec.h have their respective copyrights.

Noted, thanks. I will fix it in the next rebase. Fixed

timeyyy · 2017-01-18T20:26:40Z

Can we merge this? It's working well for me. We have a few todo such as kb_itr_interval but we can leave that for the future. I have added the license back in the extmarks branch so it will be included.

justinmk · 2017-01-18T20:35:50Z

This is still marked WIP and needs at least a rebase with the license change, plus an OK from @bfredl ...

bfredl · 2017-03-04T11:18:05Z

Sorry for the delay, I've been away from all development due to illness. This should be mostly done, I hope to fix the rebase and mark it ready "soon".

timeyyy · 2017-03-04T22:31:43Z

sorry to hear about the illness, are you back on your feet now? we missed you buddy ^^

bfredl · 2017-03-14T13:54:57Z

rebased and fixed some style issues and a memory leak.

timeyyy · 2017-03-14T15:54:48Z

will i have to apply the same fix to extended marks? If so could you mark it.

justinmk · 2017-03-14T16:04:46Z

src/nvim/lib/kbtree.h

+
+/* The following is *DEPRECATED*!!! Use the iterator interface instead! */
+
+#if 0


is this needed?

No, guess we should remove it, as we've refactored the library anyway.

bfredl · 2017-03-14T16:07:05Z

@timeyyy in extmark_free_all you will need to call kb_init and kv_init after the destroy methods just like in bufhl_clear_all, otherwise a use-after-free if a buffer is deleted and then revived.

justinmk · 2017-03-14T16:09:38Z

Would it be worth porting https://github.com/attractivechaos/klib/blob/master/test/kbtree_test.c to a lua unit test?

bfredl · 2017-03-14T16:14:31Z

That looks more like a benchmark than a unit test?

timeyyy · 2017-03-14T17:07:27Z

@bfredl what do you mean by revived? did the error only happen after using :move ?

bfredl · 2017-06-13T19:53:05Z

Hmm, I have no changes in deps. Some hidden depenency on branch name, PR number or commit metadata?

justinmk · 2017-06-13T22:52:16Z

The recent upgrade of luarocks is most likely the reason.

Cleared travis cache and restarted build.

bfredl · 2017-06-14T03:53:49Z

I had no idea Travis cached things indefinitely per PR...

justinmk · 2017-06-14T06:56:37Z

The failed travis build isn't related to this PR, it detected a loop_close hang. The QB failure also is known/unrelated.

Shougo · 2017-06-21T01:48:52Z

This feature is ready to merge?
It is needed by #5031.

bfredl · 2017-06-21T06:29:01Z

Let's make at least most of the travis builds pass again. (I given up the hope of seeing a green build anytime soon).

bfredl · 2017-06-21T16:06:04Z

The qb release build complains:

02:37:23,120 WARN  - /home/quickbuild/buildagent/workspace/root/neovim/pull-requests-automated/src/nvim/bufhl_defs.h: In function ‘kb_del_itr_bufhl’:
02:37:23,120 WARN  - /home/quickbuild/buildagent/workspace/root/neovim/pull-requests-automated/src/nvim/lib/kbtree.h:238:28: error: array subscript is below array bounds [-Werror=array-bounds]
02:37:23,120 WARN  - kp = __KB_KEY(key_t, x)[i]; \
02:37:23,120 WARN  - ^

At least the first one is clearly wrong, which makes me suspect all are. Can -Werror=array-bounds be turned off for a segment of code?

justinmk · 2017-06-21T21:39:38Z

could use a pragma, we're doing that in at least assert.h, I also temporarily used it here though that was later replaced.

bfredl · 2017-06-23T07:03:53Z

Compiles cleanly now everywhere.

@justinmk Are you planning to release 0.2.1 soon-ish? Then maybe this should wait, would be best to be used a bit on master before shipped in a release.

Shougo · 2017-06-23T07:13:46Z

I think 0.2.1 is not soon...

https://github.com/neovim/neovim/pulls?q=is%3Aopen+is%3Apr+milestone%3A0.2.1

bfredl · 2017-06-23T07:33:53Z

@Shougo IIRC for "patch" level releases, they don't need to cover everything in a milestone, they just need to be a stable enough point with some new features/bug fixes. Rather the milestone will just be renamed to 0.2.2, which as you see don't exist yet.

Shougo · 2017-06-23T07:36:23Z

OK.

justinmk · 2017-06-23T20:40:37Z

When TUI settles down 0.2.1 is probably a good idea. Soon I hope.

bfredl · 2017-06-24T09:10:13Z

Sound that will take a while. Let's merge this, if tests passes once more...

marvim added the WIP label Aug 28, 2016

bfredl force-pushed the kbtree branch from a816ba4 to d90bea8 Compare October 31, 2016 13:26

bfredl force-pushed the kbtree branch 3 times, most recently from df0bcb7 to 0cf2d28 Compare November 24, 2016 19:23

Hinidu reviewed Nov 29, 2016

View reviewed changes

bfredl force-pushed the kbtree branch 8 times, most recently from 45143eb to 41c3e0b Compare March 14, 2017 13:52

bfredl changed the title ~~[WIP] Kbtree_t~~ [RFC] add kbtree_t and use it for bufhl Mar 14, 2017

marvim added RFC and removed WIP labels Mar 14, 2017

justinmk reviewed Mar 14, 2017

View reviewed changes

bfredl force-pushed the kbtree branch 2 times, most recently from 50d63ff to 36748da Compare June 13, 2017 06:14

bfredl force-pushed the kbtree branch from 36748da to ccb77a0 Compare June 19, 2017 18:51

bfredl force-pushed the kbtree branch from ccb77a0 to 36056cd Compare June 21, 2017 06:27

bfredl force-pushed the kbtree branch from 65e04e0 to 8a44f87 Compare June 23, 2017 07:02

timeyyy and others added 8 commits June 24, 2017 11:09

kbtree.h

0b6b03c

bufhl: use kbtree for bufhl

1eff241

kbtree: allow iterators to start at arbitrary position

6712e08

kbtree: use proper structs

53cf88c

kbtree: eliminate unneccesary heap allocation

14e19b8

kbtree: make warning free and delete deprecated macros

28a549d

bufhl: some style cleanup

7873660

bufhl: fix move

8b375cf

bfredl force-pushed the kbtree branch from 8a44f87 to 8b375cf Compare June 24, 2017 09:09

bfredl merged commit ca385db into neovim:master Jun 24, 2017

jszakmeister removed the RFC label Jun 24, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RFC] add kbtree_t and use it for bufhl #5266

[RFC] add kbtree_t and use it for bufhl #5266

bfredl commented Aug 28, 2016 •

edited

timeyyy commented Oct 23, 2016

bfredl commented Oct 24, 2016

Hinidu Nov 29, 2016

bfredl Nov 30, 2016 •

edited

timeyyy commented Jan 18, 2017

justinmk commented Jan 18, 2017

bfredl commented Mar 4, 2017

timeyyy commented Mar 4, 2017

bfredl commented Mar 14, 2017

timeyyy commented Mar 14, 2017

justinmk Mar 14, 2017

bfredl Mar 14, 2017

bfredl commented Mar 14, 2017

justinmk commented Mar 14, 2017

bfredl commented Mar 14, 2017

timeyyy commented Mar 14, 2017

bfredl commented Jun 13, 2017

justinmk commented Jun 13, 2017 •

edited

bfredl commented Jun 14, 2017

justinmk commented Jun 14, 2017

Shougo commented Jun 21, 2017

bfredl commented Jun 21, 2017

bfredl commented Jun 21, 2017

justinmk commented Jun 21, 2017

bfredl commented Jun 23, 2017

Shougo commented Jun 23, 2017 •

edited

bfredl commented Jun 23, 2017

Shougo commented Jun 23, 2017

justinmk commented Jun 23, 2017

bfredl commented Jun 24, 2017


		/* The following is DEPRECATED!!! Use the iterator interface instead! */

		#if 0

[RFC] add kbtree_t and use it for bufhl #5266

[RFC] add kbtree_t and use it for bufhl #5266

Conversation

bfredl commented Aug 28, 2016 • edited

timeyyy commented Oct 23, 2016

bfredl commented Oct 24, 2016

Hinidu Nov 29, 2016

Choose a reason for hiding this comment

bfredl Nov 30, 2016 • edited

Choose a reason for hiding this comment

timeyyy commented Jan 18, 2017

justinmk commented Jan 18, 2017

bfredl commented Mar 4, 2017

timeyyy commented Mar 4, 2017

bfredl commented Mar 14, 2017

timeyyy commented Mar 14, 2017

justinmk Mar 14, 2017

Choose a reason for hiding this comment

bfredl Mar 14, 2017

Choose a reason for hiding this comment

bfredl commented Mar 14, 2017

justinmk commented Mar 14, 2017

bfredl commented Mar 14, 2017

timeyyy commented Mar 14, 2017

bfredl commented Jun 13, 2017

justinmk commented Jun 13, 2017 • edited

bfredl commented Jun 14, 2017

justinmk commented Jun 14, 2017

Shougo commented Jun 21, 2017

bfredl commented Jun 21, 2017

bfredl commented Jun 21, 2017

justinmk commented Jun 21, 2017

bfredl commented Jun 23, 2017

Shougo commented Jun 23, 2017 • edited

bfredl commented Jun 23, 2017

Shougo commented Jun 23, 2017

justinmk commented Jun 23, 2017

bfredl commented Jun 24, 2017

bfredl commented Aug 28, 2016 •

edited

bfredl Nov 30, 2016 •

edited

justinmk commented Jun 13, 2017 •

edited

Shougo commented Jun 23, 2017 •

edited