[vim/vim] Allow attaching custom data to setqflist() via 'user_data' field (PR #11818)
Tom Praschan
For example, LSP plugins could use this to attach the original LSP Location item for things like 'textDocument/reference'
You can view, comment on, or merge this pull request online at:
https://github.com/vim/vim/pull/11818
Commit Summary
- 130438b Allow attaching custom data to setqflist() via 'user_data' field
File Changes
(3 files)
- M runtime/doc/builtin.txt (2)
- M src/quickfix.c (21)
- M src/testdir/test_quickfix.vim (3)
Patch Links:
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
codecov[bot]
Codecov Report
Merging #11818 (1c242b0) into master (24a8d06) will decrease coverage by
0.80%.
The diff coverage is100.00%.
@@ Coverage Diff @@ ## master #11818 +/- ## ========================================== - Coverage 81.90% 81.09% -0.81% ========================================== Files 164 154 -10 Lines 192861 182433 -10428 Branches 43732 41307 -2425 ========================================== - Hits 157969 147951 -10018 + Misses 22102 21553 -549 - Partials 12790 12929 +139
| Flag | Coverage Δ | |
|---|---|---|
| huge-clang-none | 82.60% <100.00%> (+<0.01%) |
⬆️ |
| huge-gcc-none | ? |
|
| huge-gcc-testgui | ? |
|
| huge-gcc-unittests | 0.29% <0.00%> (-0.01%) |
⬇️ |
| linux | 81.09% <100.00%> (-1.27%) |
⬇️ |
| mingw-x64-HUGE | ? |
|
| mingw-x86-HUGE | ? |
|
| windows | ? |
Flags with carried forward coverage won't be shown. Click here to find out more.
| Impacted Files | Coverage Δ | |
|---|---|---|
| src/quickfix.c | 89.88% <100.00%> (-1.33%) |
⬇️ |
| src/if_perl.xs | 54.50% <0.00%> (-17.82%) |
⬇️ |
| src/regexp_nfa.c | 80.56% <0.00%> (-9.33%) |
⬇️ |
| src/arabic.c | 85.86% <0.00%> (-8.70%) |
⬇️ |
| src/typval.c | 82.05% <0.00%> (-8.00%) |
⬇️ |
| src/regexp_bt.c | 78.50% <0.00%> (-7.47%) |
⬇️ |
| src/vim9execute.c | 82.61% <0.00%> (-7.12%) |
⬇️ |
| src/json.c | 77.84% <0.00%> (-5.47%) |
⬇️ |
| src/vim9instr.c | 77.10% <0.00%> (-4.74%) |
⬇️ |
| src/vim9compile.c | 86.82% <0.00%> (-4.60%) |
⬇️ |
| ... and 138 more |
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Yegappan Lakshmanan
@yegappan commented on this pull request.
In src/quickfix.c:
> @@ -951,6 +952,7 @@ typedef struct {
char_u *pattern;
int enr;
int type;
+ typval_T *user_data;
The support for garbage collection needs to be added for "user_data". You can try setting a Dict as "user_data", invoke test_garbagecollect_now() function to do the garbage collection and then retrieve the user_data using getqflist() and try to access the Dict.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Tom Praschan
@tom-anders commented on this pull request.
In src/quickfix.c:
> @@ -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()...?
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Bram Moolenaar
> @tom-anders commented on this pull request.
>
> 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
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 ///
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Tom Praschan
@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
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Tom Praschan
@brammool @yegappan Does this look good or is there anything else you want me to add? Should I address the code coverage issue?
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Yegappan Lakshmanan
@yegappan commented on this pull request.
In src/quickfix.c:
> @@ -7801,6 +7819,23 @@ set_errorlist(
return retval;
}
+static int mark_quickfix_user_data(qf_info_T *qi, int copyID)
+{
+ int abort = FALSE;
+ 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.
In 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.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Bram Moolenaar
I would also expect a change in qf_free_items((). clear_tv() should be called for qf_user_data
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Tom Praschan
@tom-anders pushed 1 commit.
- 7f68ff7 Allow attaching custom data to setqflist() via 'user_data' field
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Tom Praschan
@tom-anders commented on this pull request.
In 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)
Done, sorry for missing that
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Tom Praschan
@tom-anders commented on this pull request.
In src/quickfix.c:
> @@ -7801,6 +7819,23 @@ set_errorlist(
return retval;
}
+static int mark_quickfix_user_data(qf_info_T *qi, int copyID)
+{
+ int abort = FALSE;
+ 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 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..?
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Tom Praschan
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!
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Tom Praschan
@tom-anders pushed 1 commit.
- 5c3bef8 Allow attaching custom data to setqflist() via 'user_data' field
You are receiving this because you are subscribed to this thread.
dundargoc
@dundargoc commented on this pull request.
In src/quickfix.c:
> @@ -7801,6 +7819,23 @@ set_errorlist(
return retval;
}
+static int mark_quickfix_user_data(qf_info_T *qi, int copyID)
+{
+ int abort = FALSE;
+ for (int i = 0; i < LISTCOUNT && !abort; ++i) {
+ qfline_T *qfp;
+ int j;
+ FOR_ALL_QFL_ITEMS(&qi->qf_lists[i], qfp, j)
@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?
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Yegappan Lakshmanan
@yegappan commented on this pull request.
In src/quickfix.c:
> @@ -7801,6 +7819,23 @@ set_errorlist(
return retval;
}
+static int mark_quickfix_user_data(qf_info_T *qi, int copyID)
+{
+ int abort = FALSE;
+ for (int i = 0; i < LISTCOUNT && !abort; ++i) {
+ qfline_T *qfp;
+ int j;
+ FOR_ALL_QFL_ITEMS(&qi->qf_lists[i], qfp, j)
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.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Tom Praschan
@tom-anders pushed 1 commit.
- 4a40c81 Allow attaching custom data to setqflist() via 'user_data' field
You are receiving this because you are subscribed to this thread.
Tom Praschan
@tom-anders commented on this pull request.
In src/quickfix.c:
> @@ -7801,6 +7819,23 @@ set_errorlist(
return retval;
}
+static int mark_quickfix_user_data(qf_info_T *qi, int copyID)
+{
+ int abort = FALSE;
+ for (int i = 0; i < LISTCOUNT && !abort; ++i) {
+ qfline_T *qfp;
+ int j;
+ FOR_ALL_QFL_ITEMS(&qi->qf_lists[i], qfp, j)
Thanks, I was still not sure where to add the default value, but qf_new_list looked reasonable, hope this is correct?
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Yegappan Lakshmanan
@yegappan commented on this pull request.
In src/quickfix.c:
> @@ -2173,6 +2179,12 @@ qf_add_entry(
qfp->qf_col = col;
qfp->qf_end_col = end_col;
qfp->qf_viscol = vis_col;
+ if (user_data == NULL || user_data->v_type == VAR_UNKNOWN)
+ qfp->qf_user_data.v_type = VAR_UNKNOWN;
+ else {
Can you move the curly brace to the next line (to follow the coding style)?
In src/quickfix.c:
> @@ -7801,6 +7824,27 @@ set_errorlist(
return retval;
}
+static int mark_quickfix_user_data(qf_info_T *qi, int copyID)
+{
+ int abort = FALSE;
+ for (int i = 0; i < LISTCOUNT && !abort; ++i) {
+ qf_list_T *qfl = &qi->qf_lists[i];
+ if (qfl->qf_has_user_data)
To reduce the indentation below, can you change the above to the following?
if (!qfl->qf_has_user_data)
continue;
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Yegappan Lakshmanan
> @@ -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()?
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Tom Praschan
@tom-anders pushed 1 commit.
- 897e12e Allow attaching custom data to setqflist() via 'user_data' field
You are receiving this because you are subscribed to this thread.
Tom Praschan
@tom-anders commented on this pull request.
In src/quickfix.c:
> @@ -2335,6 +2347,7 @@ copy_loclist_entries(qf_list_T *from_qfl, qf_list_T *to_qfl) from_qfp->qf_pattern,
Done 👍
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Yegappan Lakshmanan
@yegappan commented on this pull request.
In src/quickfix.c:
> @@ -2335,6 +2347,7 @@ copy_loclist_entries(qf_list_T *from_qfl, qf_list_T *to_qfl) from_qfp->qf_pattern,
I think you also need to copy the per-item user_data in the copy_loclist_entries() function?
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Yegappan Lakshmanan
@yegappan approved this pull request.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Yegappan Lakshmanan
@yegappan commented on this pull request.
The changes look good to me.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Tom Praschan
Thanks for the review and your time and patience!
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Tom Praschan
@brammool Friendly ping - is there anything stopping this from being included?
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
zeertzjq
There are some code style test failures caused by trailing white spacez.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Bram Moolenaar
It's in the todo list. I want to include bug fixes first. And please make the tests pass.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Tom Praschan
@tom-anders pushed 1 commit.
- bc72cd1 Allow attaching custom data to setqflist() via 'user_data' field
You are receiving this because you are subscribed to this thread.
Tom Praschan
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 and branch (and also test_codestyle.vim didn't even exist on my branch).
A rebase helped :)
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Yegappan Lakshmanan
Can you please update and rebase this PR to the latest?
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Tom Praschan
Sorry, I was on vacation, thanks for merging!
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.