emacs-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: noverlay branch


From: Stefan Monnier
Subject: Re: noverlay branch
Date: Sun, 09 Oct 2022 00:12:49 -0400
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/29.0.50 (gnu/linux)

> In any case, I do have a new test for tests/src/buffer-tests.el that
> crashes feature/noverlay Emacs immediately, when ENABLE_CHECKING is on,
> in a spot in itree.c having to do with offset inheritance.

(Not?) great!

> Two patches, also on https://git.sr.ht/~matta/emacs/log/feature/noverlay
> as before.

Merged (along with another that you apparently pushed since).

> I'll work on fixing the crash next, but wanted to get the
> test in because it takes an...interesting approach...that may require
> discussion.

I'm fine with the random testing approach (I'd be happy to see something
like `propcheck` used in our testing rig).
Currently, our test suite is not really setup to test Emacs crashes
very well, so it's not ideal.  Still: better than nothing.

> @@ -1086,9 +1086,17 @@ interval_tree_inherit_offset (uintmax_t otick, struct 
> interval_node *node)
>          node->right->offset += node->offset;
>        node->offset = 0;
>      }
> -  /* FIXME: I wonder when/why this condition can be false, and more generally
> -     why we'd want to propagate offsets that may not be fully up-to-date.  */
> -  if (node->parent == ITREE_NULL || node->parent->otick == otick)
> +  /* FIXME: I wonder when/why this condition can be false, and more
> +     generally why we'd want to propagate offsets that may not be
> +     fully up-to-date. --stef
> +
> +     Offsets can be inherited from dirty nodes (with out of date
> +     otick) during insert and remove.  Offsets aren't inherited
> +     downward from the root for these operations so rotations are
> +     performed on potentially "dirty" nodes.  We could fix this by
> +     always inheriting offsets downward from the root for every insert
> +     and remove.  --matt
> +  */
>      node->otick = otick;
>  }

Indeed, I saw later in the remove code that we do propagate offsets
without caring if they're up-to-date, and I think the logic behind that
is sound.  And indeed we can drop the test because the only property we
only ever care about for an `otick` is whether it's equal to
`tree->otick`, so if node->parent->otick != otick then performing the
assignment is equivalent to not performing it.


        Stefan




reply via email to

[Prev in Thread] Current Thread [Next in Thread]