[vim/vim] Large rework of how virtual text properties are stored, plus multiple other property bug fixes. (PR #19685)
Paul Ollis
My apologies for the size of this PR, but the fix for #19680 and #19681
required some major redesign/rework. During the course of those changes other
issues became apparent, which I felt could not be disentangled into separate
PRs.
The issues that this PR addresses are: #12568, #19256, #12677, #12678, #12679,
#12680, #12681, #12682, #12683 and #12684.
The major change is to store the text for virtual text properties within the
memline instead of a special table attached to the buffer. This change:
-
Makes undo/redo robust when virtual text properties are used (#19680).
-
Stops ever increasing memory usage by the table mentioned above (#19681).
-
Removes the restriction about using negative property IDs.
The changes to the property help file reflects the removal of the above
restriction and also include some corrections/clarifications.
I created quite a few additional tests in test_textprop2.vim, which uses
textprop_support.vim and textprop.py to "model" properties. The
textprop_support.vim provides a "facade" class that delegates to a class in
textprop.py.
The reason for this structure and the use of Python is quite simply that
writing the "model" using VimScript too difficult. Perhaps another person might
be able turn the "facade" in textprop_support.vim into a pure VimScript
implementation. But, sorry, it will not be me.
There are also a number of small changes to test scripts, which allowed me to
run the tests reliably on my local PC.
You can view, comment on, or merge this pull request online at:
https://github.com/vim/vim/pull/19685
Commit Summary
- 7861ca6 Store virtual text in memline.
File Changes
(35 files)
- M runtime/doc/textprop.txt (31)
- M src/buffer.c (3)
- M src/change.c (4)
- M src/charset.c (153)
- M src/drawline.c (115)
- M src/errors.h (10)
- M src/ex_cmds.c (221)
- M src/indent.c (1)
- M src/memline.c (243)
- M src/netbeans.c (2)
- M src/ops.c (62)
- M src/proto/change.pro (2)
- M src/proto/strings.pro (2)
- M src/proto/textprop.pro (27)
- M src/strings.c (2)
- M src/structs.h (110)
- M src/testdir/Make_all.mak (2)
- M src/testdir/dumps/Test_prop_in_empty_popup.dump (2)
- M src/testdir/test_cmdmods.vim (3)
- M src/testdir/test_codestyle.vim (2)
- M src/testdir/test_crash.vim (2)
- M src/testdir/test_gui.vim (4)
- M src/testdir/test_ins_complete.vim (4)
- M src/testdir/test_lispindent.vim (87)
- M src/testdir/test_search.vim (2)
- M src/testdir/test_startup.vim (16)
- M src/testdir/test_substitute.vim (2)
- M src/testdir/test_textprop.vim (136)
- A src/testdir/test_textprop2.vim (520)
- M src/testdir/test_window_cmd.vim (2)
- A src/testdir/textprop.py (1017)
- A src/testdir/textprop_support.vim (102)
- M src/testdir/util/socketserver.vim (4)
- M src/textprop.c (2656)
- M src/undo.c (35)
Patch Links:
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Paul Ollis
@paul-ollis pushed 2 commits.
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Paul Ollis
@paul-ollis pushed 1 commit.
- d947e58 Get PR tests to run clean.
You are receiving this because you are subscribed to this thread.
Foxe Chen
64-bitman left a comment (vim/vim#19685)I wonder if this conflicts with #19661, that PR only modifies the drawing code.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Paul Ollis
paul-ollis left a comment (vim/vim#19685)@64-bitman. Yes #19661 does conflict.
I am clearly some way off getting a stable PR run, but assuming both PRs are accepted then I think there will be a tricky re-base regardless of which PR get merged first.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Paul Ollis
@paul-ollis pushed 1 commit.
- 8406b9c Get PR tests to run clean.
You are receiving this because you are subscribed to this thread.
h_east
@h-east commented on this pull request.
About the document.
Write according to the rules.
:h help-writing /^STYLE
The following two points are particularly important:
Vim help files generally use 2 spaces after a sentence (since they are written
using a fixed-width font and that was the preferred style in the 70s/80s),
like what is described here: https://english.stackexchange.com/a/2602
(snip...)
Use two spaces between the final dot of a sentence of the first letter of the
next sentence.
In src/testdir/test_textprop.vim:
> @@ -3606,7 +3655,7 @@ func Test_props_with_text_below_nowrap()
vim9script
edit foobar
set nowrap
- set showbreak=+++\
+ set showbreak=+++\
This doesn't seem to be the intended fix.
In src/structs.h:
> + buf_T *buf; // [R] The line's buffer. + linenr_T lnum; // [R] The number of the line. May be zero to + // indicate that no line is loaded. + bool detached; // [R] When set, "text" and property virtual text + // is allocated. + colnr_T text_size; // [R] Size of text including NUL. + char_u *text; // [R] Just the NUL terminated text. + uint16_t prop_size; // [P] The number of allocated "props". + uint16_t prop_count; // [R] The number of properties. + textprop_T *props; // [R] Variable length vector of properties. Treat + // as read-only unless detached is true. + bool text_changed; // [P] The text has been changed. Note "detached" + // must also be set. + int adjust_flags; // Flags controlling "prev" and "next" + // adjustments. + + // Next and previous lines, used to adjust neighbouring properties.
Generally, single-sentence code comments do not contain periods. While there are no specific rules regarding this, I believe it's the same in Viim.
The same applies to code comments outside of this section.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Paul Ollis
@paul-ollis pushed 1 commit.
- f01d46c Get PR tests to run clean.
You are receiving this because you are subscribed to this thread.
Paul Ollis
@paul-ollis pushed 3 commits.
- a7e2983 Store virtual text in memline.
- 3a02f2b Get PR tests to run clean.
- 0a1a071 Get PR tests to run clean.
You are receiving this because you are subscribed to this thread.
Paul Ollis
@paul-ollis commented on this pull request.
In src/testdir/test_textprop.vim:
> @@ -3606,7 +3655,7 @@ func Test_props_with_text_below_nowrap()
vim9script
edit foobar
set nowrap
- set showbreak=+++\
+ set showbreak=+++\
My vim configuration automatically removes trailing white space "noise" because, well it is "noise" and can only confuse.
I did notice the change had crept in but (a) the test still passes and (b) there is nothing to indicate that anyone thought the space to be important.
I see no good deliberately add the space back in.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Paul Ollis
@paul-ollis commented on this pull request.
In src/structs.h:
> + buf_T *buf; // [R] The line's buffer. + linenr_T lnum; // [R] The number of the line. May be zero to + // indicate that no line is loaded. + bool detached; // [R] When set, "text" and property virtual text + // is allocated. + colnr_T text_size; // [R] Size of text including NUL. + char_u *text; // [R] Just the NUL terminated text. + uint16_t prop_size; // [P] The number of allocated "props". + uint16_t prop_count; // [R] The number of properties. + textprop_T *props; // [R] Variable length vector of properties. Treat + // as read-only unless detached is true. + bool text_changed; // [P] The text has been changed. Note "detached" + // must also be set. + int adjust_flags; // Flags controlling "prev" and "next" + // adjustments. + + // Next and previous lines, used to adjust neighbouring properties.
I try to respect the existing style of a code base but properly punctuated, single sentence comments appear
elsewhere within the code. I chose to follow that style because it fits with philosophy.
I much prefer properly punctuated comments, net least because it is respectful of the reader.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
h_east
@h-east commented on this pull request.
In src/testdir/test_textprop.vim:
> @@ -3606,7 +3655,7 @@ func Test_props_with_text_below_nowrap()
vim9script
edit foobar
set nowrap
- set showbreak=+++\
+ set showbreak=+++\
That's a bit of a stretch.
In Markdown files, removing trailing whitespace changes the layout.
Similarly, removing trailing whitespace in Vim scripts can cause unintended behavior or test failures.
Could you please revert it?
Below is a list (grep result) of lines that require trailing whitespace as intended for the test.
test_conceal.vim|151 col 24-26| set showbreak=\ >>>\
test_display.vim|345 col 21-23| set fillchars=eob:\
test_display.vim|416 col 28-30| set fillchars+=foldinner:\
test_listchars.vim|414 col 34-36| set listchars=tab:>-,leadtab:\|\
test_listchars.vim|422 col 35-37| set listchars=tab:>-,leadtab:│\
test_popupwin.vim|1168 col 19-21| set showbreak=>>\
test_scroll_opt.vim|520 col 49-51| set smoothscroll scrolloff=0 showbreak=+++\
test_signs.vim|168 col 42-44| sign define Sign5 linehl=Comment text=X\
test_textprop.vim|3609 col 24-26| set showbreak=+++\
test_textprop.vim|4276 col 20-22| set showbreak=>\
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
dkearns
@dkearns commented on this pull request.
In src/testdir/test_textprop.vim:
> @@ -3606,7 +3655,7 @@ func Test_props_with_text_below_nowrap()
vim9script
edit foobar
set nowrap
- set showbreak=+++\
+ set showbreak=+++\
The backslash in that value is only present to escape the trailing space character and include it in the value. The trailing backspace now makes no sense. Set it with :let if you like but the change is, I assume, irrelevant to the PR anyway.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Paul Ollis
@paul-ollis pushed 1 commit.
- e70e002 Get PR tests to run clean.
You are receiving this because you are subscribed to this thread.
Paul Ollis
@paul-ollis commented on this pull request.
In src/testdir/test_textprop.vim:
> @@ -3606,7 +3655,7 @@ func Test_props_with_text_below_nowrap()
vim9script
edit foobar
set nowrap
- set showbreak=+++\
+ set showbreak=+++\
@h-east and @dkearns. Thanks for your comments.
I have had a closer look at the test in question and the showbreak setting
appears to serve no purpose, except possibly to prove that showbreak has no
effect in the test's setup.
So, I have removed the "" character as well, which at prevents the test
remaining such a puzzle.
If anyone knows (or suspects) that the test is broken and should be investigated
further then please raise an issue.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Foxe Chen
@64-bitman commented on this pull request.
In src/textprop.c:
> } #endif // FEAT_PROP_POPUP +
I think these notes should be at the top of the file
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Paul Ollis
@paul-ollis commented on this pull request.
@64-bitman. An interesting comment. I actually did have the notes at the top then moved the bottom because there was a lot of comment to scroll through in order to get to the code. In my (disturbingly) long career I have encountered more people who, in such cases, prefer such details to be tucked away at the bottom of the file.
I do not have strong feelings either way (at least as far as this PR goes) and will happily follow consensus or an authoritative steer from the core team.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Paul Ollis
@paul-ollis pushed 1 commit.
- 2dc0d60 Mainly fixing AddressSanitizer errors.
You are receiving this because you are subscribed to this thread.
dkearns
@dkearns commented on this pull request.
In src/testdir/test_textprop.vim:
> @@ -3606,7 +3655,7 @@ func Test_props_with_text_below_nowrap()
vim9script
edit foobar
set nowrap
- set showbreak=+++\
+ set showbreak=+++\
From 48ca24d it would appear it's included just to confirm that it's not drawn.
You're probably now aware but +++ is a pseudo default value as it's a prominent example at :help showbreak.
However, it's inconsistently used even in this test file and there are also some other examples of trailing single backslashes, which I've always considered a rather ugly, exception to the \\ literal backslash rule.
setlocal number showbreak=+ breakindent breakindentopt=shift:2
setlocal linebreak showbreak=+ breakindent breakindentopt=shift:2
call term_sendkeys(buf, ":set showbreak=+++\<CR>")
set showbreak=+++
set showbreak=+++
set showbreak=+++
set showbreak=+++\
call term_sendkeys(buf, ":set showbreak=>>\<CR>")
call term_sendkeys(buf, ":set showbreak=\<CR>")
set showbreak=>\
set cursorline scrolloff=0 showbreak=>\ smoothscroll
But let's not derail this commentary any further...
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
zeertzjq
@zeertzjq commented on this pull request.
In src/testdir/test_ins_complete.vim:
> @@ -3311,7 +3311,7 @@ endfunc func Test_ins_complete_end_of_line() " this was reading past the end of the line new - norm 8o€ý + norm 8o€ý
This trailing space is inserted as buffer text. It should be kept.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
h_east
@h-east commented on this pull request.
In src/testdir/test_ins_complete.vim:
> @@ -3311,7 +3311,7 @@ endfunc func Test_ins_complete_end_of_line() " this was reading past the end of the line new - norm 8o€ý + norm 8o€ý
@paul-ollis
Could you please revert the following settings?
My vim configuration automatically removes trailing white space "noise" because, well it is "noise" and can only confuse.
You are modifying parts that don't need to be changed, seemingly without realizing it. It's becoming counterproductive because you're trying to apply even more changes to address the unexpected side effects I've pointed out. These lines are there for a reason and are not "noise," so please leave them as they are.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Paul Ollis
paul-ollis left a comment (vim/vim#19685)So, the CI build is stubbornly refusing to run clean because this error on "huge" builds.
Failures:
From test_textprop2.vim:
Caught exception: Vim(import):E1053: Could not import "./util/textprop_support.vim"
@ command line
script /home/runner/work/vim/vim/src/testdir/runtest.vim[567]
/home/runner/work/vim/vim/src/testdir/test_textprop2.vim, line 9
And I am frankly currently baffled. The problem does not occur for the "normal" build (the test script runs and passes fine). There should be no reason for the "huge" build to work as well. At the point of the failure, Vim is just sourcing some code and, as part of that, loading some Python 3 code. Python 3 is enabled and the Python 3 tests are running and passing.
Any ideas, suggestions or help about how to figure out the problem would be very gratefully received. I have tried to replicate the "huge" build on my local PC and am pretty sure I have succeeded with something that is very close. For me the tests run fine.
I am about to try some experimenting which will, temporarily, break other tests so I will make this PR draft for a while.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Paul Ollis
@paul-ollis pushed 2 commits.
You are receiving this because you are subscribed to this thread.
Foxe Chen
64-bitman left a comment (vim/vim#19685)So, the CI build is stubbornly refusing to run clean because this error on "huge" builds.
Failures: From test_textprop2.vim: Caught exception: Vim(import):E1053: Could not import "./util/textprop_support.vim" @ command line script /home/runner/work/vim/vim/src/testdir/runtest.vim[567] /home/runner/work/vim/vim/src/testdir/test_textprop2.vim, line 9And I am frankly currently baffled. The problem does not occur for the "normal" build (the test script runs and passes fine). There should be no reason for the "huge" build to work as well. At the point of the failure, Vim is just sourcing some code and, as part of that, loading some Python 3 code. Python 3 is enabled and the Python 3 tests are running and passing.
Any ideas, suggestions or help about how to figure out the problem would be very gratefully received. I have tried to replicate the "huge" build on my local PC and am pretty sure I have succeeded with something that is very close. For me the tests run fine.
I am about to try some experimenting which will, temporarily, break other tests so I will make this PR draft for a while.
In test_textprop2, i don't think you can use the import command in legacy vim script
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Paul Ollis
paul-ollis left a comment (vim/vim#19685)In test_textprop2, i don't think you can use the import command in legacy vim script.
Thank for the suggestion, but - no it works. The test_textprop2.vim script works absolutely fine for the "normal" build.
In fact, a bunch of (legacy vim) test scripts do "import './util/vim9.vim' as v9"
Once again thanks.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Paul Ollis
@paul-ollis commented on this pull request.
In src/textprop.c:
> } #endif // FEAT_PROP_POPUP +
It seems we have a consensus of one, so I am moving the notes to the top of the file.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Paul Ollis
@paul-ollis pushed 1 commit.
- 6f51ba7 Add missing files ...
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
@chrisbra commented on this pull request.
I found a few issues. Some are style (like braces for single statements, using the FASE/TRUE macros for booleans, using free() instead of vim_free(). Then there is at least a potential double free in prop_add_one() and the error case in perform_properties_adjustment() on um_detach() failure looks a bit suspicious.
In src/change.c:
> @@ -2562,7 +2562,7 @@ truncate_line(int fixpos)
* Saves the lines for undo first if "undo" is TRUE.
*/
void
-del_lines(long nlines, int undo)
+del_lines(long nlines,int undo, int flags)
please keep the space after nlines
In src/testdir/test_textprop.vim:
> @@ -4667,51 +4697,6 @@ func Test_text_prop_diff_mode() call StopVimInTerminal(buf) endfunc -func Test_error_when_using_negative_id()
Why is this deleted? This test and the next one should be kept, no?
In src/drawline.c:
> {
- textprop_T *tp = &text_props[used_tpi];
- char_u *p = ((char_u **)wp->w_buffer
- ->b_textprop_text.ga_data)[
- -text_prop_id - 1];
- int above = (tp->tp_flags
- & TP_FLAG_ALIGN_ABOVE);
- int bail_out = FALSE;
+ textprop_T *tp = &text_props[used_tpi];
+ char_u *p = tp->tp_id != -MAXCOL ? tp->tp_text : NULL;
+ int above = PROP_IS_ABOVE(tp);
+ int bail_out = FALSE;
bool?
In src/errors.h:
> @@ -3432,10 +3430,6 @@ EXTERN char e_class_variable_str_not_found_in_class_str[]
INIT(= N_("E1337: Class variable \"%s\" not found in class \"%s\""));
// E1338 unused
#endif
-#ifdef FEAT_PROP_POPUP
-EXTERN char e_cannot_add_textprop_with_text_after_using_textprop_with_negative_id[]
- INIT(= N_("E1339: Cannot add a textprop with text after using a textprop with a negative id"));
-#endif
can you update the comment above?
In src/ex_cmds.c:
> matchcol = (colnr_T)STRLEN(sub_firstline) - matchcol; prev_matchcol = (colnr_T)STRLEN(sub_firstline) - prev_matchcol; + linenr_T join_start_line = lnum; + linenr_T del_start_lnum = lnum + 1; + long del_count = nmatch_tl - 1; + bool start_line_is_dead = FALSE;⬇️ Suggested change
- bool start_line_is_dead = FALSE; + bool start_line_is_dead = false;
In src/ex_cmds.c:
> + linenr_T join_start_line = lnum;
+ linenr_T del_start_lnum = lnum + 1;
+ long del_count = nmatch_tl - 1;
+ bool start_line_is_dead = FALSE;
+ if (nmatch_tl == 0)
+ {
+ STRCAT(new_start, sub_firstline + copycol);
+ if (u_savesub(lnum) != OK)
+ break;
+ ml_replace(lnum, new_start, TRUE);
+ }
+ else
+ {
+ if (STRLEN(new_start) == 0)
+ {
+ start_line_is_dead = TRUE;
⬇️ Suggested change
- start_line_is_dead = TRUE; + start_line_is_dead = true;
In src/textprop.c:
> + umemline->prop_size = (uint16_t)prop_size;
+ return TRUE;
+}
+
+/*
+ * Common code for going to a specific line.
+ */
+ static bool
+um_goto_line_common(
+ unpacked_memline_T *umemline,
+ linenr_T lnum,
+ int extra_props,
+ bool copy_props)
+{
+ if (umemline->buf == NULL)
+ return FALSE;
false
In src/textprop.c:
> + for (int i = 0; i < prop_count; i++)
+ {
+ textprop_T *prop = &umemline->props[i];
+ if (prop->tp_flags & TP_FLAG_VTEXT_BITS)
+ {
+ if (prop->tp_text_offset > (colnr_T)max_valid_vtext_offset)
+ goto corrupted; // NUL char beyond end of memline!
+ prop->tp_text = count_ptr + prop->tp_text_offset;
+ }
+ else
+ {
+ prop->tp_text = NULL;
+ }
+ }
+ }
+ return TRUE;
true
In src/textprop.c:
> + prop->tp_text = count_ptr + prop->tp_text_offset;
+ }
+ else
+ {
+ prop->tp_text = NULL;
+ }
+ }
+ }
+ return TRUE;
+
+corrupted:
+ iemsg(e_text_property_info_corrupted);
+
+fail:
+ um_abort(umemline);
+ return FALSE;
false
In src/textprop.c:
> +
+/*
+ * Set the text for an unpacked memline.
+ *
+ * The provided "text" must be a pointer to allocated memory. The unpacked
+ * memline takes ownership of this allocated memory. The unpacked memline
+ * becomes detached.
+ *
+ * This returns false if the unpacked memline could not be detached. If already
+ * detached then this call cannot fail.
+ */
+ bool
+um_set_text(unpacked_memline_T *umemline, char_u *text)
+{
+ if (!umemline->detached && !um_detach(umemline))
+ return FALSE;
false and further in this file
In src/textprop.c:
> + * Set the text for an unpacked memline.
+ *
+ * The provided "text" must be a pointer to allocated memory. The unpacked
+ * memline takes ownership of this allocated memory. The unpacked memline
+ * becomes detached.
+ *
+ * This returns false if the unpacked memline could not be detached. If already
+ * detached then this call cannot fail.
+ */
+ bool
+um_set_text(unpacked_memline_T *umemline, char_u *text)
+{
+ if (!umemline->detached && !um_detach(umemline))
+ return FALSE;
+
+ free(umemline->text);
vim_free
In src/textprop.c:
> + umemline->text_changed = TRUE;
+ return TRUE;
+}
+
+ static bool
+um_add_prop_common(
+ unpacked_memline_T *umemline, const textprop_T *prop, bool sorted)
+{
+ if (!umemline->detached && !um_detach(umemline))
+ return FALSE;
+
+ if (umemline->prop_size == umemline->prop_count)
+ {
+ if (!um_add_space_for_props(umemline, 1))
+ {
+ return FALSE;
can drop the braces
In src/textprop.c:
> + *
+ * This ensures than any changes get 'written' back to the buffer's memline.
+ * The unpacked memline should not be used after this.
+ */
+ void
+um_close(unpacked_memline_T *umemline)
+{
+ if (umemline->lnum > 0)
+ um_goto_line(umemline, 0, 0);
+}
+
+/*
+ * Close an unpacked memline without saving.
+ *
+ * All memory is freed and the "buf" is set to NULL to indicate the closed
+ * state. Use this for error conditions or to close without writaing changes
⬇️ Suggested change
- * state. Use this for error conditions or to close without writaing changes + * state. Use this for error conditions or to close without writing changes
In src/textprop.c:
> +
+/*
+ * Create a neighbouring unpacked memline.
+ */
+ static unpacked_memline_T *
+um_create_neighbour(unpacked_memline_T *umemline, linenr_T lnum)
+{
+ unpacked_memline_T *neighbour = ALLOC_ONE(unpacked_memline_T);
+ if (neighbour == NULL)
+ return NULL;
+
+ *neighbour = um_open_at_detached(umemline->buf, lnum, 0);
+ if (neighbour->buf == NULL)
+ {
+ um_abort(neighbour);
+ free(neighbour);
vim_free
In src/textprop.c:
> + *
+ * Returns OK on success or if already detached and FAIL if any memory
+ * allocation fails, in which case the unpacked memline is left in an unusable
+ * state.
+ */
+ static bool
+um_detach(unpacked_memline_T *umemline)
+{
+ if (umemline->detached)
+ return TRUE;
+
+ int i = 0;
+ if (umemline->text != NULL)
+ {
+ umemline->text = vim_strsave(umemline->text);
+ }
can drop the braces again
In src/textprop.c:
> +
+ for (i = 0; i < umemline->prop_count; i++)
+ {
+ textprop_T *prop = &umemline->props[i];
+ if (prop->tp_text != NULL)
+ {
+ prop->tp_text = vim_strsave(prop->tp_text);
+ if (prop->tp_text == NULL)
+ goto fail;
+ }
+ }
+ umemline->detached = TRUE;
+ return TRUE;
+
+fail:
+ free(umemline->text);
umemline->text = NULL;
In src/textprop.c:
> + if (prop->tp_text != NULL)
+ {
+ prop->tp_text = vim_strsave(prop->tp_text);
+ if (prop->tp_text == NULL)
+ goto fail;
+ }
+ }
+ umemline->detached = TRUE;
+ return TRUE;
+
+fail:
+ free(umemline->text);
+ for (int j = 0; j < i; j++)
+ {
+ free(umemline->props[j].tp_text);
+ }
I am wondering, if we should do um_abort() here?
In src/textprop.c:
> + prop_mod_T (*adjust)(textprop_T *, colnr_T, colnr_T))
+{
+ unpacked_memline_T umemline = um_open_at(buf, lnum, 0);
+ if (umemline.buf == NULL)
+ return;
+
+ for (int i = 0; i < umemline.prop_count; i++)
+ {
+ textprop_T *prop = &umemline.props[i];
+ if (PROP_IS_FLOATING(prop))
+ continue;
+
+ int is_vtext = PROP_IS_VTEXT(prop);
+ prop_mod_T result = adjust(prop, col, count);
+ if (result != PC_UNMODIFIED && !um_detach(&umemline))
+ break;
if um_detach() fails midway in this loop, we break out and then close the inconsistent umemline.
In src/textprop.c:
> tmp_prop.tp_type = type->pt_id; tmp_prop.tp_flags = text_flags | (lnum > start_lnum ? TP_FLAG_CONT_PREV : 0) | (lnum < end_lnum ? TP_FLAG_CONT_NEXT : 0) | ((type->pt_flags & PT_FLAG_INS_START_INCL) ? TP_FLAG_START_INCL : 0); tmp_prop.tp_padleft = text_padding_left; - mch_memmove(newprops + i * sizeof(textprop_T), &tmp_prop, - sizeof(textprop_T)); - - if (i < proplen) - mch_memmove(newprops + (i + 1) * sizeof(textprop_T), - props + i * sizeof(textprop_T), - sizeof(textprop_T) * (proplen - i)); - - if (buf->b_ml.ml_flags & (ML_LINE_DIRTY | ML_ALLOCATED)) - vim_free(buf->b_ml.ml_line_ptr); - buf->b_ml.ml_line_ptr = newtext; - buf->b_ml.ml_line_len += sizeof(textprop_T); - buf->b_ml.ml_flags |= ML_LINE_DIRTY; + tmp_prop.tp_text = virt_text;
this causes a double free in case of an error.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Paul Ollis
@paul-ollis commented on this pull request.
In src/testdir/test_textprop.vim:
> @@ -4667,51 +4697,6 @@ func Test_text_prop_diff_mode() call StopVimInTerminal(buf) endfunc -func Test_error_when_using_negative_id()
Hi Chris, thanks for finding the time to review this.
Perhaps this is comment and the need to rebase the PR in light of #19741, is a
good trigger to discuss just totally simplifying positive and negative IDs.
[Please be aware that I was pretty exhausted by the time I submitted this PR.
What I expected to be a small fix turned into fixing non-trivial design flaws,
wading through technical debt and, frankly, some code that long ago became way
too hard to maintain.
By the end, the desire to keep the PR well focused conflicted with a desire to
simply get the PR submitted. I am painfully aware that I may have failed to
acheive the correct balance - for which I apologise. With that in mind...]
As I understand it, the restrictions on when negative IDs could be used were
only ever required because of the way that the text for virtual text properties
was separately stored; using negative IDs as an index. In other words, the
introduction of virtual text properties allowed implementation details to
leak into and complicate the text property API.
As a result of this PR the need for the restrictions disappeared and the
associated code covered by these tests "disolved", as I recall. The code for
using negative IDs for virtual text properties did "survive" albeit in
simplified form.
Thinking about this again, I cannot see any technical reason to place any
restrictions on IDs. That is, if the user supplies a value for the ID then
there is no technical reason that the provided ID should not be used.
The current (master) API rules feel like a bit of a mess to me. It would be
good to make them at least well defined before I rebase.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Paul Ollis
@paul-ollis commented on this pull request.
In src/change.c:
> @@ -2562,7 +2562,7 @@ truncate_line(int fixpos)
* Saves the lines for undo first if "undo" is TRUE.
*/
void
-del_lines(long nlines, int undo)
+del_lines(long nlines,int undo, int flags)
Fixed for next push.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Paul Ollis
@paul-ollis commented on this pull request.
In src/drawline.c:
> {
- textprop_T *tp = &text_props[used_tpi];
- char_u *p = ((char_u **)wp->w_buffer
- ->b_textprop_text.ga_data)[
- -text_prop_id - 1];
- int above = (tp->tp_flags
- & TP_FLAG_ALIGN_ABOVE);
- int bail_out = FALSE;
+ textprop_T *tp = &text_props[used_tpi];
+ char_u *p = tp->tp_id != -MAXCOL ? tp->tp_text : NULL;
+ int above = PROP_IS_ABOVE(tp);
+ int bail_out = FALSE;
I agree, but it is existing code. Is changing the types for "bail_out" and "above" acceptable code churn?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Paul Ollis
@paul-ollis commented on this pull request.
In src/errors.h:
> @@ -3432,10 +3430,6 @@ EXTERN char e_class_variable_str_not_found_in_class_str[]
INIT(= N_("E1337: Class variable \"%s\" not found in class \"%s\""));
// E1338 unused
#endif
-#ifdef FEAT_PROP_POPUP
-EXTERN char e_cannot_add_textprop_with_text_after_using_textprop_with_negative_id[]
- INIT(= N_("E1339: Cannot add a textprop with text after using a textprop with a negative id"));
-#endif
Done.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Paul Ollis
@paul-ollis pushed 1 commit.
- 535bac0 Apply suggestions from code review
—
View it on GitHub.
You are receiving this because you are subscribed to this thread.
Paul Ollis
@paul-ollis pushed 1 commit.
- 88bfea8 Changes from code reviewing.
—
View it on GitHub.
You are receiving this because you are subscribed to this thread.
h_east
@h-east requested changes on this pull request.
I have strong reservations about addressing multiple distinct issues/improvements within a single PR. Whether such a major logic change is truly necessary remains a matter of debate. First and foremost, each issue should be handled individually.
Furthermore, I fail to understand the refusal to comply with requests from Vim members regarding non-essential parts of the PR.
#19685 (comment)
#19685 (comment)
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
h_east
h-east left a comment (vim/vim#19685)Review result
Bugs
-
Memory leak in perform_properties_adjustment
When u_save() fails, the function returns without calling um_abort(&umemline),
leaking the detached umemline. -
Dead code in remove_bytes_1
In the virtual text branch, after last_deleted_byte >= prop->tp_col already
returns PC_RQ_DELETE, the subsequent prop->tp_col <= last_deleted_byte check
is unreachable. The prop->tp_col = col assignment never executes. -
vt_id_counter overflow
The global counter decrements monotonically with no wraparound guard. It will
hit INT_MIN and cause undefined behavior in very long-running sessions. -
Test_subst_adjusts_marks early return
There is a bare return after the first LoadEditAndCheck call, causing the
remaining 7 test cases to be silently skipped. Likely a debug leftover. -
Double free (noted by @chrisbra at line ~1318)
The error path can trigger a double free. This was flagged in the code review
but it's unclear whether it has been fully resolved.
Unused code
The vtext_T struct (with ref_count) is defined in structs.h but never
referenced anywhere in the codebase. Should be removed or deferred to when
it's actually needed.
Code style
- Numerous typos in comments: "typs", "propery", "memorry", "hae", "nolonger",
"writaing", "propbably", "responsibilty", "reomving" - Mixed bool/true/false and int/TRUE/FALSE — Vim traditionally uses the
latter. @chrisbra's review also touched on this but the direction seems
inconsistent across the PR. - ERR_FAIL/ERR_RET macros using comma operators are hard to read.
#define COPY_PROPS truestyle is unusual for the Vim codebase.
Tests
Python test model (textprop.py, 1017 lines):
- Requires CheckFeature python3, so all new tests in test_textprop2.vim are
silently skipped on builds without Python3. - This is a significant departure from Vim's self-contained testing
philosophy. - The Python code itself has bugs: dead code in test_delete (code after
return), a duplicate condition in insert_text, and incorrect type annotation
string (should be str).
Removed tests:
- Test_removed_prop_with_text_cleans_up_array,
Test_error_when_using_negative_id, Test_error_after_using_negative_id are
deleted. The removals are consistent with the design change, but there should
be a replacement test verifying that negative IDs now work without error.
Documentation
- Typo in textprop.txt: *before" should be before (missing closing
asterisk). - prop_list() sort description changed from "ordered by starting column and
priority" to "ordered by starting column" — is the removal of priority sorting
intentional?
Unrelated changes
The following are mixed into this PR but are unrelated to text property
rework. They make bisecting harder and should ideally be in a separate PR:
- g:ignoreSwapExists additions (test_cmdmods.vim, test_codestyle.vim,
test_search.vim, test_window_cmd.vim) - --noplugin additions (test_crash.vim, test_gui.vim, test_options.vim,
test_startup.vim) - TermWait timeout change (test_messages.vim)
- Trailing whitespace removal (test_ins_complete.vim)
- Typo fix (test_substitute.vim)
- Indent fix (socketserver.vim)
Summary
- Memory leak and potential double free bugs
- The early return in Test_subst_adjusts_marks skipping most test cases
- Separation of unrelated changes
- The Python test dependency question
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.