Allow attaching custom data to setqflist() via 'user_data' field #11818

tom-anders · 2023-01-14T19:43:55Z

For example, LSP plugins could use this to attach the original LSP Location item for things like 'textDocument/reference'

codecov · 2023-01-14T19:55:26Z

Codecov Report

Merging #11818 (bc72cd1) into master (3ab3a86) will increase coverage by 0.01%.
The diff coverage is 84.37%.

@@            Coverage Diff             @@
##           master   #11818      +/-   ##
==========================================
+ Coverage   82.06%   82.08%   +0.01%     
==========================================
  Files         160      160              
  Lines      193437   193469      +32     
  Branches    43433    43426       -7     
==========================================
+ Hits       158740   158801      +61     
+ Misses      21866    21827      -39     
- Partials    12831    12841      +10

Flag	Coverage Δ
huge-clang-none	`82.73% <84.37%> (+0.02%)`	⬆️
linux	`82.73% <84.37%> (+0.02%)`	⬆️
mingw-x64-HUGE	`76.56% <84.61%> (+<0.01%)`	⬆️
mingw-x86-HUGE	`77.02% <84.61%> (-0.01%)`	⬇️
windows	`78.15% <84.61%> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/quickfix.c	`91.18% <84.37%> (-0.09%)`	⬇️

... and 15 files with indirect coverage changes

src/quickfix.c

brammool · 2023-01-14T21:47:19Z

@tom-anders commented on this pull request. > @@ -951,6 +952,7 @@ typedef struct { char_u *pattern; int enr; int type; + typval_T *user_data; Yeah that indeed seems to clear the dict. How do I add it to garbage collection though? `dict_alloc()`...?

The entry point is set_ref_in_quickfix(). You can see it calls mark_quickfix_ctx() to handle "qf_ctx", something similar has to happen for all user_data values.

…

-- Me? A skeptic? I trust you have proof. /// Bram Moolenaar -- ***@***.*** -- http://www.Moolenaar.net \\\ /// \\\ \\\ sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ /// \\\ help me help AIDS victims -- http://ICCF-Holland.org ///

tom-anders · 2023-01-15T10:51:21Z

@tom-anders commented on this pull request. > @@ -951,6 +952,7 @@ typedef struct { char_u pattern; int enr; int type; + typval_T user_data; Yeah that indeed seems to clear the dict. How do I add it to garbage collection though? dict_alloc()...?
The entry point is set_ref_in_quickfix(). You can see it calls mark_quickfix_ctx() to handle "qf_ctx", something similar has to happen for all user_data values.
…
-- Me? A skeptic? I trust you have proof. /// Bram Moolenaar -- @.* -- http://www.Moolenaar.net \\ /// \\ \\ sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ /// \\ help me help AIDS victims -- http://ICCF-Holland.org ///

Thanks for the pointers, think I got it now

tom-anders · 2023-02-19T13:42:38Z

@brammool @yegappan Does this look good or is there anything else you want me to add? Should I address the code coverage issue?

yegappan · 2023-02-19T16:01:31Z

src/quickfix.c

+    for (int i = 0; i < LISTCOUNT && !abort; ++i) {
+	qfline_T *qfp;
+	int j;
+        FOR_ALL_QFL_ITEMS(&qi->qf_lists[i], qfp, j) 


This is functionally correct but inefficient. The garbage collection runs often. If a user has multiple quickfix/location lists with a large number of entries (without any user data), then this loop will slow down Vim. Is it possible to set a flag in the list if any of the item has a user data attached to it? This way only lists with items that have a user data attached need to be inspected.

This is certainly possible, but tbh I am not yet familiar enough with the code to find all the right places to set this flag - Obviously we should update the flag inside qf_add_entry(), but I'm not sure in which other places I'd need to initialize/update/copy this flag..?

@yegappan would you say this is a blocker? If so, mind giving a few pointers so this fellow can get this to the finish line?

Yes. This will slow down the garbage collection with a large number of quickfix/location lists/entries.

To address this, a flag should be added to qf_list_T that indicates whether any of the entries in that list have user data. By default, this will be set to FALSE. In the qf_add_entry() function, when copying the user data, set this flag to TRUE. When running the garbage collection for the quickfix lists, check this flag. If the flag is set, then do the garbage collection update for the user data in the quickfix items for that list. If the flag it not set, the list can be skipped.

Thanks, I was still not sure where to add the default value, but qf_new_list looked reasonable, hope this is correct?

yegappan · 2023-02-19T16:03:06Z

src/testdir/test_quickfix.vim

  let l = g:Xgetlist()
  call assert_equal(2, len(l))
  call assert_equal(2, l[1].lnum)
  call assert_equal(3, l[1].end_lnum)
  call assert_equal(4, l[1].col)
  call assert_equal(5, l[1].end_col)
+  call assert_equal({'6': [7, 8]}, l[1].user_data)



Can you add one more test for garbage collection? You can add a complex type as user data, then call the test_garbagecollect_now() function and then retrieve the user data and check to make sure that it is still present.

Done, sorry for missing that

brammool · 2023-02-19T20:33:19Z

I would also expect a change in qf_free_items((). clear_tv() should be called for qf_user_data

tom-anders · 2023-02-20T18:13:33Z

I would also expect a change in qf_free_items((). clear_tv() should be called for qf_user_data

Added clear_tv as suggested, thanks!

src/quickfix.c

yegappan · 2023-03-16T16:01:12Z

src/quickfix.c

@@ -2335,6 +2347,7 @@ copy_loclist_entries(qf_list_T *from_qfl, qf_list_T *to_qfl)
 		    from_qfp->qf_pattern,


The new flag has to be copied to the new location list in copy_loclist(). Also, do you need to copy the per quickfix item user_data in copy_loclist_entries()?

yegappan

The changes look good to me.

tom-anders · 2023-03-16T16:31:10Z

Thanks for the review and your time and patience!

tom-anders · 2023-05-06T16:18:31Z

@brammool Friendly ping - is there anything stopping this from being included?

zeertzjq · 2023-05-06T16:23:14Z

There are some code style test failures caused by trailing white spaces.

brammool · 2023-05-06T16:55:50Z

It's in the todo list. I want to include bug fixes first. And please make the tests pass.

tom-anders · 2023-05-06T17:18:19Z

Ahh, sorry about that - I think the last time I took a look at the CI, I was really confused because the line numbers wouldn't match with my local branch (and also test_codestyle.vim didn't even exist on my branch).

A rebase helped :)

yegappan · 2023-08-11T15:05:56Z

Can you please update and rebase this PR to the latest?

Problem: cannot store custom data in quickfix list Solution: add `user_data` field for the quickfix list closes: vim/vim#11818 vim/vim@ca6ac99 Co-authored-by: Tom Praschan <13141438+tom-anders@users.noreply.github.com>

tom-anders · 2023-08-20T10:38:55Z

Sorry, I was on vacation, thanks for merging!

Problem: cannot store custom data in quickfix list Solution: add `user_data` field for the quickfix list closes: vim#11818 Signed-off-by: Christian Brabandt <cb@256bit.org> Co-authored-by: Tom Praschan <13141438+tom-anders@users.noreply.github.com>

tom-anders force-pushed the qf_user_data branch from 130438b to 1c242b0 Compare January 14, 2023 19:45

yegappan reviewed Jan 14, 2023

View reviewed changes

src/quickfix.c Show resolved Hide resolved

tom-anders force-pushed the qf_user_data branch from 1c242b0 to d8f6056 Compare January 15, 2023 10:50

yegappan reviewed Feb 19, 2023

View reviewed changes

tom-anders force-pushed the qf_user_data branch from d8f6056 to 7f68ff7 Compare February 20, 2023 18:10

tom-anders force-pushed the qf_user_data branch from 7f68ff7 to 5c3bef8 Compare February 21, 2023 19:40

tom-anders mentioned this pull request Mar 15, 2023

feat(quickfix): add user_data field to quickfix items neovim/neovim#21618

Closed

tom-anders force-pushed the qf_user_data branch from 5c3bef8 to 4a40c81 Compare March 16, 2023 14:58

yegappan reviewed Mar 16, 2023

View reviewed changes

src/quickfix.c Outdated Show resolved Hide resolved

src/quickfix.c Outdated Show resolved Hide resolved

yegappan reviewed Mar 16, 2023

View reviewed changes

tom-anders force-pushed the qf_user_data branch from 4a40c81 to 897e12e Compare March 16, 2023 16:16

yegappan approved these changes Mar 16, 2023

View reviewed changes

yegappan reviewed Mar 16, 2023

View reviewed changes

Allow attaching custom data to setqflist() via 'user_data' field

bc72cd1

tom-anders force-pushed the qf_user_data branch from 897e12e to bc72cd1 Compare May 6, 2023 17:16

tom-anders mentioned this pull request May 24, 2023

feat(lsp): add original LSP Location as item's user_data in locations_to_items neovim/neovim#23743

Merged

yegappan added the release-9.1 label Aug 11, 2023

yegappan added this to the vim-9.1 milestone Aug 11, 2023

yegappan removed the release-9.1 label Aug 11, 2023

chrisbra closed this in ca6ac99 Aug 11, 2023

zeertzjq mentioned this pull request Aug 11, 2023

vim-patch:9.0.1688: cannot store custom data in quickfix list neovim/neovim#24673

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow attaching custom data to setqflist() via 'user_data' field #11818

Allow attaching custom data to setqflist() via 'user_data' field #11818

tom-anders commented Jan 14, 2023

codecov bot commented Jan 14, 2023 •

edited

brammool commented Jan 14, 2023 via email

tom-anders commented Jan 15, 2023

tom-anders commented Feb 19, 2023

yegappan Feb 19, 2023

tom-anders Feb 20, 2023

dundargoc Mar 16, 2023

yegappan Mar 16, 2023

tom-anders Mar 16, 2023

yegappan Feb 19, 2023

tom-anders Feb 20, 2023

brammool commented Feb 19, 2023

tom-anders commented Feb 20, 2023

yegappan Mar 16, 2023

tom-anders Mar 16, 2023

yegappan left a comment

tom-anders commented Mar 16, 2023

tom-anders commented May 6, 2023

zeertzjq commented May 6, 2023 •

edited

brammool commented May 6, 2023

tom-anders commented May 6, 2023 •

edited

yegappan commented Aug 11, 2023

tom-anders commented Aug 20, 2023

		@@ -2335,6 +2347,7 @@ copy_loclist_entries(qf_list_T from_qfl, qf_list_T to_qfl)
		from_qfp->qf_pattern,

Allow attaching custom data to setqflist() via 'user_data' field #11818

Allow attaching custom data to setqflist() via 'user_data' field #11818

Conversation

tom-anders commented Jan 14, 2023

codecov bot commented Jan 14, 2023 • edited

Codecov Report

brammool commented Jan 14, 2023 via email

tom-anders commented Jan 15, 2023

tom-anders commented Feb 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brammool commented Feb 19, 2023

tom-anders commented Feb 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yegappan left a comment

Choose a reason for hiding this comment

tom-anders commented Mar 16, 2023

tom-anders commented May 6, 2023

zeertzjq commented May 6, 2023 • edited

brammool commented May 6, 2023

tom-anders commented May 6, 2023 • edited

yegappan commented Aug 11, 2023

tom-anders commented Aug 20, 2023

codecov bot commented Jan 14, 2023 •

edited

zeertzjq commented May 6, 2023 •

edited

tom-anders commented May 6, 2023 •

edited